Re: [PATCH bpf v2 3/7] bpf, sockmap: zero-initialize pages allocated in bpf_msg_push_data

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [Critical] Uninitialized Variable Read / Scatterlist Corruption
--

commit e06d02e025ff8d66244e941a327109ac3cb65c1b
Author: Weiming Shi <bestswngs@gmail.com>

bpf, sockmap: zero-initialize pages allocated in bpf_msg_push_data

bpf_msg_push_data() allocates pages via alloc_pages() without
__GFP_ZERO. In the non-copy path, the entire page of uninitialized
heap content is added directly to the sk_msg scatterlist, which is
then transmitted over TCP to userspace via tcp_bpf_push(). In the
copy path, a gap of len bytes between the front and back memcpy
regions is similarly left uninitialized.

This leads to a kernel heap information leak: stale page content
including kernel pointers from the direct-map and vmemmap regions
is transmitted to userspace, which can be used to defeat KASLR.

Add __GFP_ZERO to the alloc_pages() call to ensure the allocated
page is always zeroed before it enters the scatterlist.

Link: https://lore.kernel.org/all/20260424155913.A19FDC19425@smtp.kernel.org

> diff --git a/net/core/filter.c b/net/core/filter.c
> index 3e555f276ba80..6e345ca65ca14 100644
> --- a/net/core/filter.c
> +++ b/net/core/filter.c
> @@ -2832,7 +2832,7 @@ BPF_CALL_4(bpf_msg_push_data, struct sk_msg *, msg, u32, start,
>  	if (unlikely(copy + len < copy))
>  		return -EINVAL;
>  
> -	page = alloc_pages(__GFP_NOWARN | GFP_ATOMIC | __GFP_COMP,
> +	page = alloc_pages(__GFP_NOWARN | GFP_ATOMIC | __GFP_COMP | __GFP_ZERO,
>  			   get_order(copy + len));
>  	if (unlikely(!page))
>  		return -ENOMEM;

[Severity: Critical]
This is a pre-existing issue and was not introduced by this patch, but does
this allocation use a garbage length when appending data?

Looking at bpf_msg_push_data(), if start == total_length, the loop iterating
through the scatterlist finishes with i == msg->sg.end:

net/core/filter.c:bpf_msg_push_data() {
    ...
	do {
		offset += l;
		l = sk_msg_elem(msg, i)->length;

		if (start < offset + l)
			break;
		sk_msg_iter_var_next(i);
	} while (i != msg->sg.end);
    ...
	if (!space || (space == 1 && start != offset))
		copy = msg->sg.data[i].length;

Since i == msg->sg.end, does the assignment to copy read from an unused or
stale scatterlist element, leaving it set to either 0 or garbage data?

If so, this garbage copy value is then used in the alloc_pages() call above
to calculate the allocation size.

When copy is 0, the non-copy path is taken, which can increment msg->sg.end,
wrap it around to msg->sg.start, and corrupt the ring buffer state by making
it appear empty.

When copy is a non-zero garbage value, it can corrupt the scatterlist length
later when we do:

net/core/filter.c:bpf_msg_push_data() {
    ...
		sg_set_page(&msg->sg.data[new], page, len + copy, 0);
    ...
}

Will this cause a mismatch with msg->sg.size and lead to out-of-bounds memory
accesses during transmission?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260611123538.156005-1-jiayuan.chen@linux.dev?part=3