Re: [PATCH 02/64] mac80211: Use flex-array for radiotap header bitmap

On Wed, Jul 28, 2021 at 10:35:56AM +0300, Dan Carpenter wrote:

On Tue, Jul 27, 2021 at 01:57:53PM -0700, Kees Cook wrote:

...

In preparation for FORTIFY_SOURCE performing compile-time and run-time field bounds checking for memcpy(), memmove(), and memset(), avoid intentionally writing across neighboring fields.

The it_present member of struct ieee80211_radiotap_header is treated as a flexible array (multiple u32s can be conditionally present). In order for memcpy() to reason (or really, not reason) about the size of operations against this struct, use of bytes beyond it_present need to be treated as part of the flexible array. Add a union/struct to contain the new "bitmap" member, for use with trailing presence bitmaps and arguments.

Additionally improve readability in the iterator code which walks through the bitmaps and arguments.

Signed-off-by: Kees Cook keescook@chromium.org

include/net/ieee80211_radiotap.h | 24 ++++++++++++++++++++---- net/mac80211/rx.c | 2 +- net/wireless/radiotap.c | 5 ++--- 3 files changed, 23 insertions(+), 8 deletions(-)

diff --git a/include/net/ieee80211_radiotap.h b/include/net/ieee80211_radiotap.h index c0854933e24f..101c1e961032 100644 --- a/include/net/ieee80211_radiotap.h +++ b/include/net/ieee80211_radiotap.h @@ -39,10 +39,26 @@ struct ieee80211_radiotap_header { */ __le16 it_len;

• /**

• * @it_present: (first) present word
  

• */
  

• __le32 it_present;

• union {

• /**
  

•  * @it_present: (first) present word
  

•  */
  

• __le32 it_present;
  

• struct {
  

• 	/* The compiler makes it difficult to overlap
  

• 	 * a flex-array with an existing singleton,
  

• 	 * so we're forced to add an empty named
  

• 	 * variable here.
  

• 	 */
  

• 	struct { } __unused;
  

• 	/**
  

• 	 * @bitmap: all presence bitmaps
  

• 	 */
  

• 	__le32 bitmap[];
  

• };
  

• };

} __packed;

This patch is so confusing...

Yeah, I agree. I tried a few ways, and was unhappy with all of them. :P

Btw, after the end of the __le32 data there is a bunch of other le64, u8 and le16 data so the struct is not accurate or complete.

It might be better to re-write this as something like this:

diff --git a/include/net/ieee80211_radiotap.h b/include/net/ieee80211_radiotap.h index c0854933e24f..0cb5719e9668 100644 --- a/include/net/ieee80211_radiotap.h +++ b/include/net/ieee80211_radiotap.h @@ -42,7 +42,10 @@ struct ieee80211_radiotap_header { /** * @it_present: (first) present word */

• __le32 it_present;

• struct {

• __le32 it_present;
  

• char buff[];
  

• } data;

} __packed;

Hm, yes, I can try this. I attempted something similar without the "only a struct" part; I was trying to avoid the identifier churn, but I guess seeing it again, it's not _that_ bad. :P

/* version is always 0 */ diff --git a/net/mac80211/rx.c b/net/mac80211/rx.c index 771921c057e8..9cc891364a07 100644 --- a/net/mac80211/rx.c +++ b/net/mac80211/rx.c @@ -328,7 +328,7 @@ ieee80211_add_rx_radiotap_header(struct ieee80211_local *local,

rthdr = skb_push(skb, rtap_len); memset(rthdr, 0, rtap_len - rtap.len - rtap.pad);

• it_present = &rthdr->it_present;

• it_present = (__le32 *)&rthdr->data;

Hm, interesting way to avoid angering the compiler during the later it_present++ updates. This is subtle ... a passer-by may not understand why this isn't just "it_present = &rthdr->data.it_present".

I think this is okay with a comment added. I'll give this a spin. Thanks!

-Kees

/* radiotap header, set always present flags */ rthdr->it_len = cpu_to_le16(rtap_len); @@ -372,7 +372,7 @@ ieee80211_add_rx_radiotap_header(struct ieee80211_local *local, ieee80211_calculate_rx_timestamp(local, status, mpdulen, 0), pos);

• rthdr->it_present |= cpu_to_le32(1 << IEEE80211_RADIOTAP_TSFT);
  

• rthdr->data.it_present |= cpu_to_le32(1 << IEEE80211_RADIOTAP_TSFT);
  

  pos += 8; }

@@ -396,7 +396,7 @@ ieee80211_add_rx_radiotap_header(struct ieee80211_local *local, *pos = 0; } else { int shift = 0;

• rthdr->it_present |= cpu_to_le32(1 << IEEE80211_RADIOTAP_RATE);
  

• rthdr->data.it_present |= cpu_to_le32(1 << IEEE80211_RADIOTAP_RATE);
  

  if (status->bw == RATE_INFO_BW_10) shift = 1; else if (status->bw == RATE_INFO_BW_5)

@@ -432,7 +432,7 @@ ieee80211_add_rx_radiotap_header(struct ieee80211_local *local, if (ieee80211_hw_check(&local->hw, SIGNAL_DBM) && !(status->flag & RX_FLAG_NO_SIGNAL_VAL)) { *pos = status->signal;

• rthdr->it_present |=
  

• rthdr->data.it_present |=
  cpu_to_le32(1 << IEEE80211_RADIOTAP_DBM_ANTSIGNAL);
  

  pos++; }

@@ -459,7 +459,7 @@ ieee80211_add_rx_radiotap_header(struct ieee80211_local *local, if (status->encoding == RX_ENC_HT) { unsigned int stbc;

• rthdr->it_present |= cpu_to_le32(1 << IEEE80211_RADIOTAP_MCS);
  

• rthdr->data.it_present |= cpu_to_le32(1 << IEEE80211_RADIOTAP_MCS);
  

  *pos++ = local->hw.radiotap_mcs_details; *pos = 0; if (status->enc_flags & RX_ENC_FLAG_SHORT_GI)

@@ -482,7 +482,7 @@ ieee80211_add_rx_radiotap_header(struct ieee80211_local *local, /* ensure 4 byte alignment */ while ((pos - (u8 *)rthdr) & 3) pos++;

• rthdr->it_present |=
  

• rthdr->data.it_present |=
  cpu_to_le32(1 << IEEE80211_RADIOTAP_AMPDU_STATUS);
  

  put_unaligned_le32(status->ampdu_reference, pos); pos += 4;

@@ -510,7 +510,7 @@ ieee80211_add_rx_radiotap_header(struct ieee80211_local *local, if (status->encoding == RX_ENC_VHT) { u16 known = local->hw.radiotap_vht_details;

• rthdr->it_present |= cpu_to_le32(1 << IEEE80211_RADIOTAP_VHT);
  

• rthdr->data.it_present |= cpu_to_le32(1 << IEEE80211_RADIOTAP_VHT);
  

  put_unaligned_le16(known, pos); pos += 2; /* flags */

@@ -553,7 +553,7 @@ ieee80211_add_rx_radiotap_header(struct ieee80211_local *local, u16 accuracy = 0; u8 flags = IEEE80211_RADIOTAP_TIMESTAMP_FLAG_32BIT;

• rthdr->it_present |=
  

• rthdr->data.it_present |=
  cpu_to_le32(1 << IEEE80211_RADIOTAP_TIMESTAMP);
  
  

  /* ensure 8 byte alignment */

@@ -642,7 +642,7 @@ ieee80211_add_rx_radiotap_header(struct ieee80211_local *local, /* ensure 2 byte alignment */ while ((pos - (u8 *)rthdr) & 1) pos++;

• rthdr->it_present |= cpu_to_le32(1 << IEEE80211_RADIOTAP_HE);
  

• rthdr->data.it_present |= cpu_to_le32(1 << IEEE80211_RADIOTAP_HE);
  

  memcpy(pos, &he, sizeof(he)); pos += sizeof(he); }

@@ -652,13 +652,13 @@ ieee80211_add_rx_radiotap_header(struct ieee80211_local *local, /* ensure 2 byte alignment */ while ((pos - (u8 *)rthdr) & 1) pos++;

• rthdr->it_present |= cpu_to_le32(1 << IEEE80211_RADIOTAP_HE_MU);
  

• rthdr->data.it_present |= cpu_to_le32(1 << IEEE80211_RADIOTAP_HE_MU);
  

  memcpy(pos, &he_mu, sizeof(he_mu)); pos += sizeof(he_mu); }

  if (status->flag & RX_FLAG_NO_PSDU) {

• rthdr->it_present |=
  

• rthdr->data.it_present |=
  cpu_to_le32(1 << IEEE80211_RADIOTAP_ZERO_LEN_PSDU);
  

  *pos++ = status->zero_length_psdu_type; }

@@ -667,7 +667,7 @@ ieee80211_add_rx_radiotap_header(struct ieee80211_local *local, /* ensure 2 byte alignment */ while ((pos - (u8 *)rthdr) & 1) pos++;

• rthdr->it_present |= cpu_to_le32(1 << IEEE80211_RADIOTAP_LSIG);
  

• rthdr->data.it_present |= cpu_to_le32(1 << IEEE80211_RADIOTAP_LSIG);
  

  memcpy(pos, &lsig, sizeof(lsig)); pos += sizeof(lsig); }

diff --git a/net/wireless/radiotap.c b/net/wireless/radiotap.c index 36f1b59a78bf..f7852024c011 100644 --- a/net/wireless/radiotap.c +++ b/net/wireless/radiotap.c @@ -114,11 +114,10 @@ int ieee80211_radiotap_iterator_init( iterator->_rtheader = radiotap_header; iterator->_max_length = get_unaligned_le16(&radiotap_header->it_len); iterator->_arg_index = 0;

• iterator->_bitmap_shifter = get_unaligned_le32(&radiotap_header->it_present);

• iterator->_bitmap_shifter = get_unaligned_le32(&radiotap_header->data.it_present); iterator->_arg = (uint8_t *)radiotap_header + sizeof(*radiotap_header); iterator->_reset_on_ext = 0;

• iterator->_next_bitmap = &radiotap_header->it_present;
• iterator->_next_bitmap++;

• iterator->_next_bitmap = (__le32 *)&radiotap_header->data.buff; iterator->_vns = vns; iterator->current_namespace = &radiotap_ns; iterator->is_radiotap_ns = 1;