[PATCH bpf v2 0/2] bpf: Fix overlapping copies in bpf_dynptr_copy()

[PATCH bpf v2 0/2] bpf: Fix overlapping copies in bpf_dynptr_copy()

[Ayushman Rout]

bpf_dynptr_copy() can corrupt data when both ranges are in the same xdp
packet and overlap: a slice of one range is used as the buffer for
__bpf_dynptr_read() or __bpf_dynptr_write(), and the chunk loop always
copies forward.

Patch 1 does not use a slice as the buffer for a copy within one packet
and copies from the end when the destination is above the source.
Patch 2 adds selftests. They pass with the fix.

Changes in v2:
- Dropped the bpf_xdp_copy_buf() change and fixed everything in
  bpf_dynptr_copy(), as suggested by Alexei
- Reworked the tests into bpf_dynptr_copy() tests where only one of the
  ranges crosses into frag0

v1: https://lore.kernel.org/bpf/20261024092659.76422-1-ayushmanrout27@gmail.com/

Ayushman Rout (2):
  bpf: Fix bpf_dynptr_copy() for overlapping ranges in one packet
  selftests/bpf: Test overlapping dynptr copies on xdp frags

 kernel/bpf/helpers.c                          | 16 +++-
 .../testing/selftests/bpf/prog_tests/dynptr.c |  3 +
 .../selftests/bpf/progs/dynptr_success.c      | 90 +++++++++++++++++++
 3 files changed, 105 insertions(+), 4 deletions(-)

-- 
2.54.0

[PATCH bpf v2 1/2] bpf: Fix bpf_dynptr_copy() for overlapping ranges in one packet

[Ayushman Rout]

When both dynptrs point into the same packet and the ranges overlap,
bpf_dynptr_copy() can copy wrong data in two ways.

If one range fits into a segment and the other one does not, the slice
of the first range is used as the buffer for __bpf_dynptr_read() or
__bpf_dynptr_write(). The range that crosses into a frag is copied piece
by piece, and an earlier piece can overwrite bytes that a later piece
has yet to read.

If neither range fits into a segment, the chunks are copied from the
start to the end through a 256 byte stack buffer. When the destination
is above the source, a chunk written to the destination overwrites
source bytes that later chunks have yet to read. For a copy of 1200
bytes with the destination 100 bytes above the source, the first chunk
writes source bytes 100..355 and the second chunk then reads bytes
256..355 after they were overwritten.

Do not use a slice as the buffer when both dynptrs share the same data,
and copy the chunks from the end to the start when the destination is
above the source, like memmove() does.

The selftests added later in this series pass with this change.

Fixes: daec295a7094 ("bpf/helpers: Introduce bpf_dynptr_copy kfunc")
Suggested-by: Alexei Starovoitov <ast@kernel.org>
Signed-off-by: Ayushman Rout <ayushmanrout27@gmail.com>
---
 kernel/bpf/helpers.c | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c
index 648abe1ba63a..b74c03669b5f 100644
--- a/kernel/bpf/helpers.c
+++ b/kernel/bpf/helpers.c
@@ -3291,8 +3291,10 @@ __bpf_kfunc int bpf_dynptr_copy(const struct bpf_dynptr *dst_ptr, u64 dst_off,
 {
 	const struct bpf_dynptr_kern *dst = (struct bpf_dynptr_kern *)dst_ptr;
 	const struct bpf_dynptr_kern *src = (struct bpf_dynptr_kern *)src_ptr;
+	bool same = src->data == dst->data;
 	void *src_slice, *dst_slice;
 	char buf[256];
+	bool rev;
 	u64 off;
 
 	src_slice = bpf_dynptr_slice(src_ptr, src_off, NULL, size);
@@ -3303,25 +3305,31 @@ __bpf_kfunc int bpf_dynptr_copy(const struct bpf_dynptr *dst_ptr, u64 dst_off,
 		return 0;
 	}
 
-	if (src_slice)
+	/* The other range is copied piece by piece. A slice of the same packet
+	 * may overlap with it, so it cannot be the buffer for that.
+	 */
+	if (src_slice && !same)
 		return __bpf_dynptr_write(dst, dst_off, src_slice, size, 0);
 
-	if (dst_slice)
+	if (dst_slice && !same)
 		return __bpf_dynptr_read(dst_slice, size, src, src_off, 0);
 
 	if (bpf_dynptr_check_off_len(dst, dst_off, size) ||
 	    bpf_dynptr_check_off_len(src, src_off, size))
 		return -E2BIG;
 
+	/* like memmove(): from the end if the destination is above the source */
+	rev = same && dst->offset + dst_off > src->offset + src_off;
 	off = 0;
 	while (off < size) {
 		u64 chunk_sz = min_t(u64, sizeof(buf), size - off);
+		u64 pos = rev ? size - off - chunk_sz : off;
 		int err;
 
-		err = __bpf_dynptr_read(buf, chunk_sz, src, src_off + off, 0);
+		err = __bpf_dynptr_read(buf, chunk_sz, src, src_off + pos, 0);
 		if (err)
 			return err;
-		err = __bpf_dynptr_write(dst, dst_off + off, buf, chunk_sz, 0);
+		err = __bpf_dynptr_write(dst, dst_off + pos, buf, chunk_sz, 0);
 		if (err)
 			return err;
 
-- 
2.54.0

[PATCH bpf v2 2/2] selftests/bpf: Test overlapping dynptr copies on xdp frags

[Ayushman Rout]

Add dynptr tests for copies within one xdp packet with frags, where the
source and the destination overlap and the destination is above the
source:

- test_dynptr_copy_xdp_overlap: both ranges cross from the linear part
  into frag0. bpf_dynptr_copy() of 1200 bytes with the destination 100
  bytes above the source.
- test_dynptr_copy_xdp_overlap_src_linear: the source is in the linear
  part and the destination crosses into frag0.
- test_dynptr_copy_xdp_overlap_dst_frag: the source crosses into frag0
  and the destination is in frag0.

The new tests set err when the data is wrong. The existing
test_dynptr_copy_xdp does not, so it can pass on corrupted data.

Without the fix to bpf_dynptr_copy(), test_dynptr_copy_xdp_overlap
fails. With it, all the new tests pass.

Signed-off-by: Ayushman Rout <ayushmanrout27@gmail.com>
---
 .../testing/selftests/bpf/prog_tests/dynptr.c |  3 +
 .../selftests/bpf/progs/dynptr_success.c      | 90 +++++++++++++++++++
 2 files changed, 93 insertions(+)

diff --git a/tools/testing/selftests/bpf/prog_tests/dynptr.c b/tools/testing/selftests/bpf/prog_tests/dynptr.c
index 4396560365e8..cbb2cc9969be 100644
--- a/tools/testing/selftests/bpf/prog_tests/dynptr.c
+++ b/tools/testing/selftests/bpf/prog_tests/dynptr.c
@@ -22,6 +22,9 @@ static struct {
 	{"test_dynptr_data", SETUP_SYSCALL_SLEEP},
 	{"test_dynptr_copy", SETUP_SYSCALL_SLEEP},
 	{"test_dynptr_copy_xdp", SETUP_XDP_PROG},
+	{"test_dynptr_copy_xdp_overlap", SETUP_XDP_PROG},
+	{"test_dynptr_copy_xdp_overlap_src_linear", SETUP_XDP_PROG},
+	{"test_dynptr_copy_xdp_overlap_dst_frag", SETUP_XDP_PROG},
 	{"test_dynptr_memset_zero", SETUP_SYSCALL_SLEEP},
 	{"test_dynptr_memset_notzero", SETUP_SYSCALL_SLEEP},
 	{"test_dynptr_memset_zero_offset", SETUP_SYSCALL_SLEEP},
diff --git a/tools/testing/selftests/bpf/progs/dynptr_success.c b/tools/testing/selftests/bpf/progs/dynptr_success.c
index b668ebd61fc7..56be4eafc270 100644
--- a/tools/testing/selftests/bpf/progs/dynptr_success.c
+++ b/tools/testing/selftests/bpf/progs/dynptr_success.c
@@ -759,6 +759,96 @@ int test_dynptr_copy_xdp(struct xdp_md *xdp)
 	return XDP_DROP;
 }
 
+static __always_inline int fill_xdp(struct bpf_dynptr *ptr, __u32 start, __u32 n)
+{
+	__u8 b;
+	int i;
+
+	bpf_for(i, 0, n) {
+		b = (start + i) % 251;
+		if (bpf_dynptr_write(ptr, start + i, &b, 1, 0))
+			return 1;
+	}
+	return 0;
+}
+
+static __always_inline int check_xdp(struct bpf_dynptr *ptr, __u32 at, __u32 n, __u32 from)
+{
+	__u8 b;
+	int i;
+
+	bpf_for(i, 0, n) {
+		if (bpf_dynptr_read(&b, 1, ptr, at + i, 0))
+			return 1;
+		if (b != (__u8)((from + i) % 251))
+			return 1;
+	}
+	return 0;
+}
+
+SEC("?xdp")
+int test_dynptr_copy_xdp_overlap(struct xdp_md *xdp)
+{
+	void *data_end = (void *)(long)xdp->data_end;
+	void *data = (void *)(long)xdp->data;
+	__u32 off = data_end - data - 1000;
+	struct bpf_dynptr ptr;
+
+	bpf_dynptr_from_xdp(xdp, 0, &ptr);
+	if (fill_xdp(&ptr, off, 1300)) {
+		err = 1;
+		return XDP_DROP;
+	}
+	err = bpf_dynptr_copy(&ptr, off + 100, &ptr, off, 1200);
+	if (err)
+		return XDP_DROP;
+	if (check_xdp(&ptr, off + 100, 1200, off))
+		err = 2;
+	return XDP_DROP;
+}
+
+SEC("?xdp")
+int test_dynptr_copy_xdp_overlap_src_linear(struct xdp_md *xdp)
+{
+	void *data_end = (void *)(long)xdp->data_end;
+	void *data = (void *)(long)xdp->data;
+	__u32 lin = data_end - data;
+	struct bpf_dynptr ptr;
+
+	bpf_dynptr_from_xdp(xdp, 0, &ptr);
+	if (fill_xdp(&ptr, lin - 64, 118)) {
+		err = 1;
+		return XDP_DROP;
+	}
+	err = bpf_dynptr_copy(&ptr, lin - 10, &ptr, lin - 64, 64);
+	if (err)
+		return XDP_DROP;
+	if (check_xdp(&ptr, lin - 10, 64, lin - 64))
+		err = 2;
+	return XDP_DROP;
+}
+
+SEC("?xdp")
+int test_dynptr_copy_xdp_overlap_dst_frag(struct xdp_md *xdp)
+{
+	void *data_end = (void *)(long)xdp->data_end;
+	void *data = (void *)(long)xdp->data;
+	__u32 lin = data_end - data;
+	struct bpf_dynptr ptr;
+
+	bpf_dynptr_from_xdp(xdp, 0, &ptr);
+	if (fill_xdp(&ptr, lin - 10, 84)) {
+		err = 1;
+		return XDP_DROP;
+	}
+	err = bpf_dynptr_copy(&ptr, lin + 10, &ptr, lin - 10, 64);
+	if (err)
+		return XDP_DROP;
+	if (check_xdp(&ptr, lin + 10, 64, lin - 10))
+		err = 2;
+	return XDP_DROP;
+}
+
 char memset_zero_data[] = "data to be zeroed";
 
 SEC("?tp/syscalls/sys_enter_nanosleep")
-- 
2.54.0

Re: [PATCH bpf v2 1/2] bpf: Fix bpf_dynptr_copy() for overlapping ranges in one packet

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] bpf: non-standard multi-line comment style in bpf_dynptr_copy()
--

commit 4a57e7196e830451e2770433571b9c37faffc18b
Author: Ayushman Rout <ayushmanrout27@gmail.com>

bpf: Fix bpf_dynptr_copy() for overlapping ranges in one packet

This commit fixes overlapping copies within the same packet in
bpf_dynptr_copy() by avoiding direct slice buffers when both dynptrs share the
same underlying data and copying chunks in reverse when the destination is
above the source.

> diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c
> index 648abe1ba63ae..b74c03669b5fd 100644
> --- a/kernel/bpf/helpers.c
> +++ b/kernel/bpf/helpers.c
> @@ -3291,8 +3291,10 @@ __bpf_kfunc int bpf_dynptr_copy(const struct bpf_dynptr *dst_ptr, u64 dst_off,
>  {
>  	const struct bpf_dynptr_kern *dst = (struct bpf_dynptr_kern *)dst_ptr;
>  	const struct bpf_dynptr_kern *src = (struct bpf_dynptr_kern *)src_ptr;
> +	bool same = src->data == dst->data;
>  	void *src_slice, *dst_slice;
>  	char buf[256];
> +	bool rev;
>  	u64 off;
>  
>  	src_slice = bpf_dynptr_slice(src_ptr, src_off, NULL, size);
> @@ -3303,25 +3305,31 @@ __bpf_kfunc int bpf_dynptr_copy(const struct bpf_dynptr *dst_ptr, u64 dst_off,
>  		return 0;
>  	}
>  
> -	if (src_slice)
> +	/* The other range is copied piece by piece. A slice of the same packet
> +	 * may overlap with it, so it cannot be the buffer for that.
> +	 */

[Severity: Low]
This isn't a bug, but in bpf_dynptr_copy() in kernel/bpf/helpers.c, could this
new multi-line comment follow the BPF subsystem comment style with the opening
/* on its own line:

	/*
	 * The other range is copied piece by piece. A slice of the same packet
	 * may overlap with it, so it cannot be the buffer for that.
	 */

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261026162810.12862-1-ayushmanrout27@gmail.com?part=1