Re: [PATCH bpf] bpf, sockmap: Fix wrong rsge offset in bpf_msg_push_data()

On 26-04-24 15:59, [email protected] wrote:
> Sashiko AI review found 4 potential issue(s):
> - [Critical] When appending data to a full or nearly full sk_msg, bpf_msg_push_data() reads the length of the wrong scatterlist element, leading to a buffer overflow or ring corruption.
> - [Critical] Missing bounds check on the 'len' parameter causes an integer overflow and a massive Out-Of-Bounds write.
> - [Critical] Scatterlist shifts fail to update the msg->sg.copy bitmap, allowing unauthorized modification of shared pages.
> - [High] Uninitialized kernel memory allocation in bpf_msg_push_data() leads to an Information Leak.
> --
> 
> commit 15a0d48ce1bdcc5941167a3e0c2a258bb5038277
> Author: Weiming Shi <[email protected]>
> 
> bpf, sockmap: Fix wrong rsge offset in bpf_msg_push_data()
> 
> When bpf_msg_push_data() splits a scatterlist element into head and
> tail, the tail's page offset is advanced by start instead of
> start - offset. This overshoot points to the wrong location within
> the page or beyond its boundary.
> 
> > diff --git a/net/core/filter.c b/net/core/filter.c
> > index 5fa9189eb772b2..a7a9ea4cb2f9a4 100644
> > --- a/net/core/filter.c
> > +++ b/net/core/filter.c
> > @@ -2860,7 +2860,7 @@ BPF_CALL_4(bpf_msg_push_data, struct sk_msg *, msg, u32, start,
> >  
> >  		psge->length = start - offset;
> >  		rsge.length -= psge->length;
> > -		rsge.offset += start;
> > +		rsge.offset += start - offset;
> >  
> >  		sk_msg_iter_var_next(i);
> >  		sg_unmark_end(psge);
> 
> This isn't a bug introduced by this patch, but looking at the earlier part
> of bpf_msg_push_data(), is the len parameter bounds-checked?
> 
> net/core/filter.c:bpf_msg_push_data() {
> ...
> 	if (!space || (space == 1 && start != offset))
> 		copy = msg->sg.data[i].length;
> 
> 	page = alloc_pages(__GFP_NOWARN | GFP_ATOMIC | __GFP_COMP,
> 			   get_order(copy + len));
> ...
> 		if (back) {
> 			from += front;
> 			to = raw + front + len;
> 
> 			memcpy(to, from, back);
> 		}
> ...
> }
> 
> If a large value is provided for len (e.g., 0xFFFFFFFF), could copy + len
> overflow 32-bit arithmetic, resulting in a very small allocation?
> 
> Later, when calculating to = raw + front + len, does the zero-extended len
> advance the destination pointer out-of-bounds, causing a massive memory
> corruption during memcpy()?
> 
> Additionally, bpf_msg_push_data() uses alloc_pages() without the
> __GFP_ZERO flag. After copying the split element's front and back parts,
> do the middle len bytes remain uninitialized?
> 
> Could this allow uninitialized kernel heap data to be added to the sk_msg
> and leaked over the network when the message is transmitted?
> 
> Also, when appending data (start == msg->sg.size), the search loop terminates
> with i == msg->sg.end. If space <= 1, the code executes
> copy = msg->sg.data[i].length.
> 
> Does this read from an unused slot?
> 
> If the slot has a garbage length from a previous pop or shift, could copy
> take that value, causing alloc_pages() to allocate a smaller size and
> leading to a kernel heap buffer overflow during the subsequent
> memcpy(raw, from, front)?
> 
> And if the garbage length is 0, could it skip the if (copy) block and
> increment msg->sg.end without space, corrupting the scatterlist ring array?
> 
> Finally, when scatterlist elements are shifted in bpf_msg_pop_data():
> 
> net/core/filter.c:sk_msg_shift_left() {
> ...
> 	do {
> 		prev = i;
> 		sk_msg_iter_var_next(i);
> 		msg->sg.data[prev] = msg->sg.data[i];
> 	} while (i != msg->sg.end);
> ...
> }
> 
> Should the msg->sg.copy bitmap also be shifted?
> 
> If a shared page (where the copy bit is 1) is shifted into a slot where
> the bit is 0, will it be falsely identified as a private page?
> 
> Could a subsequent bpf_msg_pull_data() then pass the
> !test_bit(i, msg->sg.copy) check and return a direct writable pointer to
> the shared page, enabling unauthorized modification of read-only memory?
> 
> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1

We verified the findings locally with QEMU. Two of the four issues are
confirmed with reproducers:

- copy + len integer overflow leading to heap buffer overflow [1]
- uninitialized memory leak via alloc_pages without __GFP_ZERO [2]

Patches sent for both.
[1] https://lore.kernel.org/all/[email protected]/
[2] https://lore.kernel.org/all/[email protected]/