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.
