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;
--
Kees Cook
