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

From: Kees Cook
Date: Wed Jul 28 2021 - 17:21:07 EST

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@xxxxxxxxxxxx>
> > ---
> > 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;

--
Kees Cook