| Message ID | 20210727205855.411487-3-keescook@chromium.org (mailing list archive) |
|---|---|
| State | Not Applicable |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | Introduce strict memcpy() bounds checking | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Guessing tree name failed - patch did not apply |
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... 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; /* 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; /* 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;
On Wed, Jul 28, 2021 at 10:35:56AM +0300, Dan Carpenter wrote: > @@ -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); A drive-by comment, not related to the patchset, but rather the ieee80211 driver itself. Shift expressions with (1 << NUMBER) can be subtly broken once the NUMBER is 31 and the value gets silently cast to a 64bit type. It will become 0xfffffffff80000000. I've checked the IEEE80211_RADIOTAP_* defintions if this is even remotely possible and yes, IEEE80211_RADIOTAP_EXT == 31. Fortunatelly it seems to be used with used with a 32bit types (eg. _bitmap_shifter) so there are no surprises. The recommended practice is to always use unsigned types for shifts, so "1U << ..." at least.
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;
On Wed, Jul 28, 2021 at 11:23:23AM +0200, David Sterba wrote: > On Wed, Jul 28, 2021 at 10:35:56AM +0300, Dan Carpenter wrote: > > @@ -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); > > A drive-by comment, not related to the patchset, but rather the > ieee80211 driver itself. > > Shift expressions with (1 << NUMBER) can be subtly broken once the > NUMBER is 31 and the value gets silently cast to a 64bit type. It will > become 0xfffffffff80000000. > > I've checked the IEEE80211_RADIOTAP_* defintions if this is even remotely > possible and yes, IEEE80211_RADIOTAP_EXT == 31. Fortunatelly it seems to > be used with used with a 32bit types (eg. _bitmap_shifter) so there are > no surprises. > > The recommended practice is to always use unsigned types for shifts, so > "1U << ..." at least. Ah, good catch! I think just using BIT() is the right replacement here, yes? I suppose that should be a separate patch.
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... Right, unfortunately your patch doesn't work under the strict memcpy(). :( Here are the constraints I navigated to come to the original patch I sent: * I need to directly reference a flexible array for the it_present pointer because pos is based on it, and the compiler thinks pos walks off the end of the struct: In function 'fortify_memcpy_chk', inlined from 'ieee80211_add_rx_radiotap_header' at net/mac80211/rx.c:652:3: ./include/linux/fortify-string.h:285:4: warning: call to '__write_overflow_field' declared with attribute warning: detected write beyond size of field (1st parameter); maybe use struct_group()? [-Wattribute-warning] 285 | __write_overflow_field(); | ^~~~~~~~~~~~~~~~~~~~~~~~ * It's churn/fragile to change the sizeof(), so I can't just do: - __le32 it_present; + __le32 it_bitmap[]; * I want to use a union: - __le32 it_present; + union { + __le32 it_present; + __le32 it_bitmap[]; + }; * ... but I can't actually use a union because of compiler constraints on flexible array members: ./include/net/ieee80211_radiotap.h:50:10: error: flexible array member in union 50 | __le32 it_optional[]; | ^~~~~~~~~~~ * So I came to the horrible thing I original sent. :P If I could escape the __le32 *it_present incrementing, I could use a simple change: __le32 it_present; + __le32 it_optional[]; > 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. Hm, docs seem to indicate that the packet format is multiples of u32? *shrug* Hmpf. -Kees
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: > > [...] > > - /** > > - * @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... > > 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; Ah-ha, got it: diff --git a/include/net/ieee80211_radiotap.h b/include/net/ieee80211_radiotap.h index c0854933e24f..6b7274edb3c6 100644 --- a/include/net/ieee80211_radiotap.h +++ b/include/net/ieee80211_radiotap.h @@ -43,6 +43,10 @@ struct ieee80211_radiotap_header { * @it_present: (first) present word */ __le32 it_present; + /** + * @it_optional: all remaining presence bitmaps + */ + __le32 it_optional[]; } __packed; /* version is always 0 */ diff --git a/net/mac80211/rx.c b/net/mac80211/rx.c index 2563473b5cf1..b6a960d37278 100644 --- a/net/mac80211/rx.c +++ b/net/mac80211/rx.c @@ -359,7 +359,13 @@ ieee80211_add_rx_radiotap_header(struct ieee80211_local *local, put_unaligned_le32(it_present_val, it_present); - pos = (void *)(it_present + 1); + /* + * This references through an offset into it_optional[] rather + * than via it_present otherwise later uses of pos will cause + * the compiler to think we have walked past the end of the + * struct member. + */ + pos = (void *)&rthdr->it_optional[it_present - rthdr->it_optional]; /* the order of the following fields is important */ diff --git a/net/wireless/radiotap.c b/net/wireless/radiotap.c index 36f1b59a78bf..081f0a3bdfe1 100644 --- a/net/wireless/radiotap.c +++ b/net/wireless/radiotap.c @@ -115,10 +115,9 @@ int ieee80211_radiotap_iterator_init( 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->_arg = (uint8_t *)radiotap_header + sizeof(*radiotap_header); + iterator->_arg = (uint8_t *)radiotap_header->it_optional; iterator->_reset_on_ext = 0; - iterator->_next_bitmap = &radiotap_header->it_present; - iterator->_next_bitmap++; + iterator->_next_bitmap = radiotap_header->it_optional; iterator->_vns = vns; iterator->current_namespace = &radiotap_ns; iterator->is_radiotap_ns = 1;
On Wed, Jul 28, 2021 at 04:33:18PM -0700, Kees Cook wrote: > > Ah-ha, got it: > Thanks, Kees! Nice! regards, dan carpenter
On Wed, Jul 28, 2021 at 02:54:52PM -0700, Kees Cook wrote: > On Wed, Jul 28, 2021 at 11:23:23AM +0200, David Sterba wrote: > > On Wed, Jul 28, 2021 at 10:35:56AM +0300, Dan Carpenter wrote: > > > @@ -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); > > > > A drive-by comment, not related to the patchset, but rather the > > ieee80211 driver itself. > > > > Shift expressions with (1 << NUMBER) can be subtly broken once the > > NUMBER is 31 and the value gets silently cast to a 64bit type. It will > > become 0xfffffffff80000000. > > > > I've checked the IEEE80211_RADIOTAP_* defintions if this is even remotely > > possible and yes, IEEE80211_RADIOTAP_EXT == 31. Fortunatelly it seems to > > be used with used with a 32bit types (eg. _bitmap_shifter) so there are > > no surprises. > > > > The recommended practice is to always use unsigned types for shifts, so > > "1U << ..." at least. > > Ah, good catch! I think just using BIT() is the right replacement here, > yes? I suppose that should be a separate patch. I found definition of BIT in vdso/bits.h, that does not sound like a standard header, besides that it shifts 1UL, that may not be necessary everywhere. IIRC there were objections against using the macro at all. Looking for all the definitions, there are a few that are wrong in the sense they're using the singed type, eg. https://elixir.bootlin.com/linux/v5.14-rc3/source/arch/arm/mach-davinci/sleep.S#L7 #define BIT(nr) (1 << (nr)) ... #define DEEPSLEEP_SLEEPENABLE_BIT BIT(31) but that's an assembly file so the C integer promotions don't apply. https://elixir.bootlin.com/linux/v5.14-rc3/source/drivers/staging/rtl8723bs/include/osdep_service.h#L18 https://elixir.bootlin.com/linux/v5.14-rc3/source/drivers/staging/rtl8723bs/include/wifi.h#L15 https://elixir.bootlin.com/linux/v5.14-rc3/source/tools/perf/util/intel-pt-decoder/intel-pt-pkt-decoder.c#L15 #define BIT(x) (1 << (x)) Auditing and cleaning that up is for another series, yeah, I'm just pointing it here if somebody feels like doing the work. It's IMO low hanging fruit but can reveal real bugs.
On Thu, Jul 29, 2021 at 12:45:47PM +0200, David Sterba wrote: > On Wed, Jul 28, 2021 at 02:54:52PM -0700, Kees Cook wrote: > > On Wed, Jul 28, 2021 at 11:23:23AM +0200, David Sterba wrote: > > > On Wed, Jul 28, 2021 at 10:35:56AM +0300, Dan Carpenter wrote: > > > > @@ -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); > > > > > > A drive-by comment, not related to the patchset, but rather the > > > ieee80211 driver itself. > > > > > > Shift expressions with (1 << NUMBER) can be subtly broken once the > > > NUMBER is 31 and the value gets silently cast to a 64bit type. It will > > > become 0xfffffffff80000000. > > > > > > I've checked the IEEE80211_RADIOTAP_* defintions if this is even remotely > > > possible and yes, IEEE80211_RADIOTAP_EXT == 31. Fortunatelly it seems to > > > be used with used with a 32bit types (eg. _bitmap_shifter) so there are > > > no surprises. > > > > > > The recommended practice is to always use unsigned types for shifts, so > > > "1U << ..." at least. > > > > Ah, good catch! I think just using BIT() is the right replacement here, > > yes? I suppose that should be a separate patch. > > I found definition of BIT in vdso/bits.h, that does not sound like a > standard header, besides that it shifts 1UL, that may not be necessary > everywhere. IIRC there were objections against using the macro at all. 3945ff37d2f4 ("linux/bits.h: Extract common header for vDSO") moved it there from linux/bits.h, and linux/bits.h now includes vdso/bits.h, so it is still ever-present. :)
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; /* version is always 0 */ diff --git a/net/mac80211/rx.c b/net/mac80211/rx.c index 2563473b5cf1..0d959a98e908 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 = rthdr->bitmap; /* radiotap header, set always present flags */ rthdr->it_len = cpu_to_le16(rtap_len); diff --git a/net/wireless/radiotap.c b/net/wireless/radiotap.c index 36f1b59a78bf..9f4f1a772964 100644 --- a/net/wireless/radiotap.c +++ b/net/wireless/radiotap.c @@ -115,10 +115,9 @@ int ieee80211_radiotap_iterator_init( 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->_arg = (uint8_t *)radiotap_header + sizeof(*radiotap_header); + iterator->_arg = (uint8_t *)&radiotap_header->bitmap[1]; iterator->_reset_on_ext = 0; - iterator->_next_bitmap = &radiotap_header->it_present; - iterator->_next_bitmap++; + iterator->_next_bitmap = &radiotap_header->bitmap[1]; iterator->_vns = vns; iterator->current_namespace = &radiotap_ns; iterator->is_radiotap_ns = 1;
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(-)