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