Re: [PATCH bpf v3 3/3] bpf: Fix stackmap overflow check on 32-bit arches

["Toke Høiland-Jørgensen" <toke@redhat.com>]

Bui Quang Minh <minhquangbui99@gmail.com> writes:

> On 3/7/24 19:03, Toke Høiland-Jørgensen wrote:
>> The stackmap code relies on roundup_pow_of_two() to compute the number
>> of hash buckets, and contains an overflow check by checking if the
>> resulting value is 0. However, on 32-bit arches, the roundup code itself
>> can overflow by doing a 32-bit left-shift of an unsigned long value,
>> which is undefined behaviour, so it is not guaranteed to truncate
>> neatly. This was triggered by syzbot on the DEVMAP_HASH type, which
>> contains the same check, copied from the hashtab code.
>> 
>> The commit in the fixes tag actually attempted to fix this, but the fix
>> did not account for the UB, so the fix only works on CPUs where an
>> overflow does result in a neat truncation to zero, which is not
>> guaranteed. Checking the value before rounding does not have this
>> problem.
>> 
>> Fixes: 6183f4d3a0a2 ("bpf: Check for integer overflow when using roundup_pow_of_two()")
>> Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com>
>> ---
>>   kernel/bpf/stackmap.c | 9 ++++++---
>>   1 file changed, 6 insertions(+), 3 deletions(-)
>> 
>> diff --git a/kernel/bpf/stackmap.c b/kernel/bpf/stackmap.c
>> index dff7ba539701..c99f8e5234ac 100644
>> --- a/kernel/bpf/stackmap.c
>> +++ b/kernel/bpf/stackmap.c
>> @@ -91,11 +91,14 @@ static struct bpf_map *stack_map_alloc(union bpf_attr *attr)
>>   	} else if (value_size / 8 > sysctl_perf_event_max_stack)
>>   		return ERR_PTR(-EINVAL);
>>   
>> -	/* hash table size must be power of 2 */
>> -	n_buckets = roundup_pow_of_two(attr->max_entries);
>> -	if (!n_buckets)
>> +	/* hash table size must be power of 2; roundup_pow_of_two() can overflow
>> +	 * into UB on 32-bit arches, so check that first
>> +	 */
>> +	if (attr->max_entries > 1UL << 31)
>>   		return ERR_PTR(-E2BIG);
>>   
>> +	n_buckets = roundup_pow_of_two(attr->max_entries);
>> +
>>   	cost = n_buckets * sizeof(struct stack_map_bucket *) + sizeof(*smap);
>>   	smap = bpf_map_area_alloc(cost, bpf_map_attr_numa_node(attr));
>>   	if (!smap)
>
> Reviewed-by: Bui Quang Minh <minhquangbui99@gmail.com>
>
> Today I learned to be more careful with UB in C.

Haha, yeah, I was surprised about this one as well; UB is nasty! :)

-Toke