Re: [PATCH bpf-next v2] bpf: Use memmove() to load bytes from an skb and an xdp_buff

[bot+bpf-ci@kernel.org]

[-- Attachment #1: Type: text/plain, Size: 2488 bytes --]

> bpf: Use memmove() to load bytes from an skb and an xdp_buff

TL;DR: memmove() fix leaves the multi-frag xdp path (bpf_xdp_copy_buf) using
forward memcpy(), so bpf_dynptr_read() into an overlapping rdwr slice may
still corrupt data; the store path looks similar.

> diff --git a/net/core/filter.c b/net/core/filter.c
> --- a/net/core/filter.c
> +++ b/net/core/filter.c

[ ... ]

> @@ -4298,7 +4298,7 @@ BPF_CALL_4(bpf_xdp_load_bytes, struct xdp_buff *, xdp, u32, offset,
>  	if (!ptr)
>  		bpf_xdp_copy_buf(xdp, offset, buf, len, false);
>  	else
> -		memcpy(buf, ptr, len);
> +		memmove(buf, ptr, len);
>
>  	return 0;
>  }

The commit message says "A read that spans xdp frags still goes through
bpf_xdp_copy_buf() and is not changed here."

Can the same corruption still happen on that path?

bpf_xdp_copy_buf() copies segment by segment and writes into buf as it goes:

    memcpy(dst, src, copy_len);

For an xdp dynptr, bpf_dynptr_slice_rdwr() with a NULL buffer returns a
direct pointer into whichever frag holds the requested range, so buf can
still be a slice of the same packet.

Consider a linear part of 100 bytes and a 4096 byte frag0:

    dst = bpf_dynptr_slice_rdwr(&ptr, 110, NULL, 64);   /* frag0 + 10 */
    bpf_dynptr_read(dst, 64, &ptr, 90, 0);

    bpf_dynptr_read()
      __bpf_dynptr_read()
        __bpf_xdp_load_bytes()
          bpf_xdp_load_bytes()
            bpf_xdp_pointer(xdp, 90, 64)   /* NULL, since 90 + 64 > 100 */
            bpf_xdp_copy_buf(xdp, 90, dst, 64, false)

The first memcpy() copies linear[90..100) onto frag0[10..20), which
clobbers source bytes that have not been read yet.

The next memcpy() then copies frag0[0..54) onto frag0[20..74), a forward
copy over an overlapping range.

A user-space model of this loop with forward copying returned 44 of 64 bytes
wrong.

The skb side does not look affected, since skb slices are always in the
linear head and skb_copy_bits() copies into a destination below the source.

Would it make sense to make the multi-segment copy overlap safe, for example
by going through a bounce buffer, or else to say in the commit message that
the xdp frag case remains broken?  The store path
(bpf_xdp_store_bytes() -> bpf_xdp_copy_buf(..., true)) has the same shape.


---
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/39772397846