[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <875xy6ayvb.fsf@toke.dk>
Date: Fri, 01 Mar 2024 14:02:48 +0100
From: Toke Høiland-Jørgensen <toke@...hat.com>
To: 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
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.
-Toke
[0] https://wiki.sei.cmu.edu/confluence/display/c/int34-c.+do+not+shift+an+expression+by+a+negative+number+of+bits+or+by+greater+than+or+equal+to+the+number+of+bits+that+exist+in+the+operand
Powered by blists - more mailing lists