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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 29 Jul 2021 12:45:47 +0200
From:   David Sterba <dsterba@...e.cz>
To:     Kees Cook <keescook@...omium.org>
Cc:     Dan Carpenter <dan.carpenter@...cle.com>,
        linux-hardening@...r.kernel.org,
        "Gustavo A. R. Silva" <gustavoars@...nel.org>,
        Keith Packard <keithpac@...zon.com>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Andrew Morton <akpm@...ux-foundation.org>,
        linux-kernel@...r.kernel.org, linux-wireless@...r.kernel.org,
        netdev@...r.kernel.org, dri-devel@...ts.freedesktop.org,
        linux-staging@...ts.linux.dev, linux-block@...r.kernel.org,
        linux-kbuild@...r.kernel.org, clang-built-linux@...glegroups.com
Subject: 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:
> > 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.