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

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:

quoted

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

quoted

@@ -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.