[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <87zfvj8tiz.fsf@toke.dk>
Date: Thu, 29 Feb 2024 11:16:36 +0100
From: Toke Høiland-Jørgensen <toke@...hat.com>
To: 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:
>> 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.
>>
>> Syzbot managed to turn this into a crash on arm32 by creating a DEVMAP_HASH with
>> max_entries > 0x80000000 and then trying to update it. Fix this by moving the
>> overflow check to before the rounding up operation.
>>
>> Fixes: 6f9d451ab1a3 ("xdp: Add devmap_hash map type for looking up devices by hashed index")
>> Link: https://lore.kernel.org/r/000000000000ed666a0611af6818@google.com
>> Reported-and-tested-by: syzbot+8cd36f6b65f3cafd400a@...kaller.appspotmail.com
>> Signed-off-by: Toke Høiland-Jørgensen <toke@...hat.com>
>> ---
>> kernel/bpf/devmap.c | 8 +++-----
>> 1 file changed, 3 insertions(+), 5 deletions(-)
>>
>> diff --git a/kernel/bpf/devmap.c b/kernel/bpf/devmap.c
>> index a936c704d4e7..9b2286f9c6da 100644
>> --- a/kernel/bpf/devmap.c
>> +++ b/kernel/bpf/devmap.c
>> @@ -130,13 +130,11 @@ static int dev_map_init_map(struct bpf_dtab *dtab, union bpf_attr *attr)
>> bpf_map_init_from_attr(&dtab->map, attr);
>>
>> if (attr->map_type == BPF_MAP_TYPE_DEVMAP_HASH) {
>> - dtab->n_buckets = roundup_pow_of_two(dtab->map.max_entries);
>> -
>> - if (!dtab->n_buckets) /* Overflow check */
>> + if (dtab->map.max_entries > U32_MAX / 2)
>> return -EINVAL;
>> - }
>>
>> - if (attr->map_type == BPF_MAP_TYPE_DEVMAP_HASH) {
>> + dtab->n_buckets = roundup_pow_of_two(dtab->map.max_entries);
>> +
>> dtab->dev_index_head = dev_map_create_hash(dtab->n_buckets,
>> dtab->map.numa_node);
>> if (!dtab->dev_index_head)
>> --
>> 2.43.2
>>
>
> I'm fairly sure this code was just taken from the hashtab implementation.
Yup, it was :)
> Do we also need a fix there?
>
> /* hash table size must be power of 2 */
> htab->n_buckets = roundup_pow_of_two(htab->map.max_entries);
>
> The u32 check in hashtab is,
>
> /* prevent zero size kmalloc and check for u32 overflow */
> if (htab->n_buckets == 0 ||
> htab->n_buckets > U32_MAX / sizeof(struct bucket))
> goto free_htab;
Yeah, I don't see any reason why this wouldn't be susceptible to the
same issue. I'll send a follow-up to apply the same fix there.
-Toke
Powered by blists - more mailing lists