Re: [PATCH 1/2] bpf: Fix queue/stack map u32 index overflow

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [Medium] When `attr->max_entries` is exactly `U32_MAX` and `attr->value_size` is 1, the internal map capacity tracker `qs->size` silently truncates to 0, permanently breaking the map while still allocating 4GB of kernel memory.
--

--- Patch [1]: [PATCH 1/2] bpf: Fix queue/stack map u32 index overflow ---
commit 47dbebc907a9c220d2feebeadd6501394e5a0348
Author: Yuan Chen <[email protected]>
Subject: bpf: Fix queue/stack map u32 index overflow

The queue/stack map addresses elements[] with the product of a u32
head/tail index and value_size, but the storage itself is allocated in
64-bit arithmetic. This patch restores a bound check to reject maps whose
element storage would exceed U32_MAX bytes.

> --- a/kernel/bpf/queue_stack_maps.c
> +++ b/kernel/bpf/queue_stack_maps.c
> @@ -59,6 +59,13 @@ static int queue_stack_map_alloc_check(union bpf_attr *attr)
>  		 */
>  		return -E2BIG;
>  
> +	/*
> +	 * The u32 head/tail index is multiplied by value_size to address
> +	 * elements[]. Bound the map size so the product cannot overflow.
> +	 */
> +	if ((u64)attr->max_entries * attr->value_size > U32_MAX)
> +		return -E2BIG;
> +

[Severity: Medium]
This is a pre-existing issue, but could this check still allow a truncation
in qs->size when attr->max_entries is exactly U32_MAX and attr->value_size
is 1?

If a map is created with those attributes, it bypasses the bounds check
above. Then in queue_stack_map_alloc(), the size is incremented by 1:

kernel/bpf/queue_stack_maps.c:queue_stack_map_alloc() {
    ...
    u64 size, queue_size, cost;
    ...
    size = attr->max_entries + 1;
    ...
    qs->size = size;
    ...
}

Because qs->size is declared as a u32 in struct bpf_queue_stack:

    u32 size; /* max_entries + 1 */

does assigning size (which is U32_MAX + 1) cause qs->size to silently
truncate to 0? This appears to result in a 4GB map allocation that remains
permanently empty since its capacity tracker is 0.

>  	return 0;
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1