netdev - RE: [PATCH bpf] bpf: Fix DEVMAP_HASH overflow check on 32-bit arches

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] [day] [month] [year] [list]

Message-ID: <65e20ee234bbe_5dcfe208a0@john.notmuch>
Date: Fri, 01 Mar 2024 09:22:42 -0800
From: John Fastabend <john.fastabend@...il.com>
To: Toke Høiland-Jørgensen <toke@...hat.com>, 
 John Fastabend <john.fastabend@...il.com>, 
 John Fastabend <john.fastabend@...il.com>, 
 Alexei Starovoitov <ast@...nel.org>, 
 Daniel Borkmann <daniel@...earbox.net>, 
 "David S. Miller" <davem@...emloft.net>, 
 Jakub Kicinski <kuba@...nel.org>, 
 Jesper Dangaard Brouer <hawk@...nel.org>, 
 John Fastabend <john.fastabend@...il.com>, 
 Andrii Nakryiko <andrii@...nel.org>, 
 Martin KaFai Lau <martin.lau@...ux.dev>, 
 Song Liu <song@...nel.org>, 
 Yonghong Song <yonghong.song@...ux.dev>, 
 KP Singh <kpsingh@...nel.org>, 
 Stanislav Fomichev <sdf@...gle.com>, 
 Hao Luo <haoluo@...gle.com>, 
 Jiri Olsa <jolsa@...nel.org>
Cc: syzbot+8cd36f6b65f3cafd400a@...kaller.appspotmail.com, 
 netdev@...r.kernel.org, 
 bpf@...r.kernel.org
Subject: RE: [PATCH bpf] bpf: Fix DEVMAP_HASH overflow check on 32-bit arches

Toke Høiland-Jørgensen wrote:
> John Fastabend <john.fastabend@...il.com> writes:
> 
> > Toke Høiland-Jørgensen wrote:
> >> John Fastabend <john.fastabend@...il.com> writes:
> >> 
> >> > Toke Høiland-Jørgensen wrote:
> >> >> The devmap code allocates a number hash buckets equal to the next power of two
> >> >> of the max_entries value provided when creating the map. When rounding up to the
> >> >> next power of two, the 32-bit variable storing the number of buckets can
> >> >> overflow, and the code checks for overflow by checking if the truncated 32-bit value
> >> >> is equal to 0. However, on 32-bit arches the rounding up itself can overflow
> >> >> mid-way through, because it ends up doing a left-shift of 32 bits on an unsigned
> >> >> long value. If the size of an unsigned long is four bytes, this is undefined
> >> >> behaviour, so there is no guarantee that we'll end up with a nice and tidy
> >> >> 0-value at the end.
> >
> > Hi Toke, dumb question where is this left-shift noted above? It looks
> > like fls_long tries to account by having a check for sizeof(l) == 4.
> > I'm asking mostly because I've found a few more spots without this
> > check.
> 
> That check in fls_long only switches between too different
> implementations of the fls op itself (fls() vs fls64()). AFAICT this is
> mostly meaningful for the generic (non-ASM) version that iterates over
> the bits instead of just emitting a single instruction.
> 
> The shift is in the caller:
> 
> static inline __attribute__((const))
> unsigned long __roundup_pow_of_two(unsigned long n)
> {
> 	return 1UL << fls_long(n - 1);
> }
> 
> If this is called with a value > 0x80000000, fls_long() will (correctly)
> return 32, leading to the ub[0] shift when sizeof(unsigned long) == 4.

Yep thanks I was looking in fls_long there walked past the pow-of_two()
bits. Thanks.