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

>     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. When max_entries * value_size exceeds U32_MAX, the
>     product wraps and push/peek/pop operate on the wrong element, corrupting
>     map data and leaking stale values to user space.
>
>     The original bound check was removed by commit c85d69135a91 ("bpf: move
>     memory size checks to bpf_map_charge_init()"), which migrated only the
>     bytes-to-pages conversion and dropped the overflow guard, so oversized
>     queue/stack maps can be created again.
>
>     Restore the bound in queue_stack_map_alloc_check(): reject maps whose
>     element storage would exceed U32_MAX bytes, keeping the u32 index
>     multiplication overflow-free. Also reject max_entries == U32_MAX, which
>     would make the u32 capacity counter qs->size (max_entries + 1) wrap to
>     0 and permanently break the map.
>
>     Fixes: c85d69135a91 ("bpf: move memory size checks to bpf_map_charge_init()")
>     Signed-off-by: Yuan Chen <[email protected]>

Is the attribution to c85d69135a91 right here?  The changelog says that
commit "migrated only the bytes-to-pages conversion and dropped the
overflow guard", but it looks like the guard was moved rather than
dropped.

It removed the check from queue_stack_map_alloc():

kernel/bpf/queue_stack_maps.c:queue_stack_map_alloc() {
	cost = queue_size = sizeof(*qs) + size * attr->value_size;
-	if (cost >= U32_MAX - PAGE_SIZE)
-		return ERR_PTR(-E2BIG);
-
-	cost = round_up(cost, PAGE_SIZE) >> PAGE_SHIFT;

	ret = bpf_map_charge_init(&mem, cost);
}

and the same commit added the equivalent check inside the callee:

kernel/bpf/syscall.c:bpf_map_charge_init() {
-int bpf_map_charge_init(struct bpf_map_memory *mem, u32 pages)
+int bpf_map_charge_init(struct bpf_map_memory *mem, size_t size)
 {
+	u32 pages = round_up(size, PAGE_SIZE) >> PAGE_SHIFT;
 	...
+	if (size >= U32_MAX - PAGE_SIZE)
+		return -E2BIG;
}

Since queue_stack_map_alloc() kept passing the full
sizeof(*qs) + (max_entries + 1) * value_size as cost, wouldn't oversized
queue and stack maps still have been rejected with -E2BIG right after
c85d69135a91?

The bound instead looks lost in a37fb7ef24a4 ("bpf: Eliminate
rlimit-based memory accounting for queue_stack_maps maps"), which deleted
the charge call entirely:

kernel/bpf/queue_stack_maps.c:queue_stack_map_alloc() {
-	ret = bpf_map_charge_init(&mem, cost);
-	if (ret < 0)
-		return ERR_PTR(ret);

	qs = bpf_map_area_alloc(queue_size, numa_node);
}

That leaves bpf_map_area_alloc() as the only limit on queue_size, and it
only rejects size >= SIZE_MAX on 64-bit.  Should the tag read:

  Fixes: a37fb7ef24a4 ("bpf: Eliminate rlimit-based memory accounting for queue_stack_maps maps")

with the middle paragraph reworded to match?

The difference also changes which trees the fix lands in.  As written the
tag points stable tooling at v5.3 and the v5.3..v5.10 trees, where the
check was still effective through bpf_map_charge_init(), while the
affected range appears to start at v5.11.

Given that the wrong element is addressed on push, peek and pop from
v5.11 onward, would a Cc: [email protected] line be appropriate as
well?


---
AI reviewed your patch. Please fix the bug or email reply why it's not a bug.
See: https://github.com/kernel-patches/vmtest/blob/master/ci/claude/README.md

CI run summary: https://github.com/kernel-patches/bpf/actions/runs/31375986836