[PATCH bpf 1/3] bpf: Fix bpf_xdp_copy_buf() when the buffer overlaps the packet

[PATCH bpf 1/3] bpf: Fix bpf_xdp_copy_buf() when the buffer overlaps the packet

[Ayushman Rout]

bpf_xdp_copy_buf() moves data between a caller buffer and an xdp_buff
one segment at a time (the linear part, then each frag) with memcpy().
bpf_xdp_load_bytes(), bpf_xdp_store_bytes() and bpf_dynptr_slice() with
a buffer all use it.

The buffer does not have to be outside the packet. A program can pass a
slice of the same packet, for example one from bpf_dynptr_slice_rdwr().
Then a piece that is copied early can overwrite bytes that a later piece
still has to read. memcpy() is also not valid for overlapping memory.

Example: a 100 byte linear part, a 4096 byte frag0, a buffer at
frag0 + 10 and a 64 byte load from offset 90. The first piece copies
linear[90..100) to frag0[10..20), which destroys data that the second
piece needs from frag0[0..54). A store fails in the same way.

Commit ab08863c58b1 ("bpf: Use memmove() to store bytes in an
xdp_buff") fixed the case where the range is inside one segment. A
range that crosses from the linear part into a frag still goes through
here and is not covered.

Use memmove() for every piece. For a load, copy the piece that is in the
same segment as the buffer first. For a store, copy it last. This
assumes that the frags do not overlap each other.

The selftests added later in this series fail without this patch.

Fixes: 2012c867c800 ("bpf: verifier: Accept dynptr mem as mem in helpers")
Signed-off-by: Ayushman Rout <ayushmanrout27@gmail.com>
Assisted-by: LLM
---
 net/core/filter.c | 59 ++++++++++++++++++++++++++++-------------------
 1 file changed, 35 insertions(+), 24 deletions(-)

diff --git a/net/core/filter.c b/net/core/filter.c
index 09c60a58c82a..716efafe7e0a 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -4210,47 +4210,58 @@ static const struct bpf_func_proto bpf_xdp_adjust_head_proto = {
 void bpf_xdp_copy_buf(struct xdp_buff *xdp, unsigned long off,
 		      void *buf, unsigned long len, bool flush)
 {
-	unsigned long ptr_len, ptr_off = 0;
+	unsigned long ptr_len, ptr_off, o, l;
 	skb_frag_t *next_frag, *end_frag;
 	struct skb_shared_info *sinfo;
+	u8 *ptr_buf, *b, *orig = buf;
 	void *src, *dst;
-	u8 *ptr_buf;
+	int pass;
 
 	if (likely(xdp->data_end - xdp->data >= off + len)) {
 		src = flush ? buf : xdp->data + off;
 		dst = flush ? xdp->data + off : buf;
-		memcpy(dst, src, len);
+		memmove(dst, src, len);
 		return;
 	}
 
 	sinfo = xdp_get_shared_info_from_buff(xdp);
 	end_frag = &sinfo->frags[sinfo->nr_frags];
-	next_frag = &sinfo->frags[0];
 
-	ptr_len = xdp->data_end - xdp->data;
-	ptr_buf = xdp->data;
+	for (pass = 0; pass < 2; pass++) {
+		o = off;
+		l = len;
+		b = buf;
+		ptr_off = 0;
+		ptr_len = xdp->data_end - xdp->data;
+		ptr_buf = xdp->data;
+		next_frag = &sinfo->frags[0];
+
+		while (true) {
+			if (o < ptr_off + ptr_len) {
+				unsigned long copy_off = o - ptr_off;
+				unsigned long copy_len = min(l, ptr_len - copy_off);
+				bool self = orig >= ptr_buf && orig < ptr_buf + ptr_len;
+				bool turn = pass == flush;
+
+				if (self == turn) {
+					src = flush ? b : ptr_buf + copy_off;
+					dst = flush ? ptr_buf + copy_off : b;
+					memmove(dst, src, copy_len);
+				}
 
-	while (true) {
-		if (off < ptr_off + ptr_len) {
-			unsigned long copy_off = off - ptr_off;
-			unsigned long copy_len = min(len, ptr_len - copy_off);
+				o += copy_len;
+				l -= copy_len;
+				b += copy_len;
+			}
 
-			src = flush ? buf : ptr_buf + copy_off;
-			dst = flush ? ptr_buf + copy_off : buf;
-			memcpy(dst, src, copy_len);
+			if (!l || next_frag == end_frag)
+				break;
 
-			off += copy_len;
-			len -= copy_len;
-			buf += copy_len;
+			ptr_off += ptr_len;
+			ptr_buf = skb_frag_address(next_frag);
+			ptr_len = skb_frag_size(next_frag);
+			next_frag++;
 		}
-
-		if (!len || next_frag == end_frag)
-			break;
-
-		ptr_off += ptr_len;
-		ptr_buf = skb_frag_address(next_frag);
-		ptr_len = skb_frag_size(next_frag);
-		next_frag++;
 	}
 }
 
-- 
2.54.0

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

[Ayushman Rout]

When neither dynptr can be sliced, which happens when both ranges cross
a segment boundary, bpf_dynptr_copy() copies through a 256 byte stack
buffer. It copies the chunks from the start to the end.

If both dynptrs point into the same packet, the ranges overlap and the
destination is above the source, a chunk written to the destination can
overwrite source bytes that a later chunk has not read yet.

Example: a 1200 byte copy with the destination 100 bytes above the
source. The first chunk writes source bytes 100..355, so the second
chunk reads bytes 256..355 after they were overwritten.

In that case, copy the chunks from the end to the start. This only
applies when src->data == dst->data and the destination is above the
source. All other copies stay as they are.

The selftests added later in this series fail without this patch.

Fixes: daec295a7094 ("bpf/helpers: Introduce bpf_dynptr_copy kfunc")
Signed-off-by: Ayushman Rout <ayushmanrout27@gmail.com>
Assisted-by: LLM
---
 kernel/bpf/helpers.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c
index 648abe1ba63a..8eb600cc9d12 100644
--- a/kernel/bpf/helpers.c
+++ b/kernel/bpf/helpers.c
@@ -3293,6 +3293,7 @@ __bpf_kfunc int bpf_dynptr_copy(const struct bpf_dynptr *dst_ptr, u64 dst_off,
 	const struct bpf_dynptr_kern *src = (struct bpf_dynptr_kern *)src_ptr;
 	void *src_slice, *dst_slice;
 	char buf[256];
+	bool rev;
 	u64 off;
 
 	src_slice = bpf_dynptr_slice(src_ptr, src_off, NULL, size);
@@ -3313,15 +3314,17 @@ __bpf_kfunc int bpf_dynptr_copy(const struct bpf_dynptr *dst_ptr, u64 dst_off,
 	    bpf_dynptr_check_off_len(src, src_off, size))
 		return -E2BIG;
 
+	rev = src->data == dst->data && 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 3/3] selftests/bpf: Test overlapping dynptr copies on xdp frags

[Ayushman Rout]

Add four dynptr tests. Three use an xdp packet with frags, and in each
the range goes from the linear part into frag0:

- test_dynptr_copy_xdp_overlap: bpf_dynptr_copy() of 1200 bytes with
  the destination 100 bytes above the source.
- test_dynptr_read_xdp_overlap: bpf_dynptr_read() into a slice of the
  same packet.
- test_dynptr_write_xdp_overlap: bpf_dynptr_write() from a slice of the
  same packet.

The fourth, test_dynptr_skb_load_overlap, reads a range that spans the
head and the frags of an skb. It passes with and without the fixes and
checks that skbs are not affected.

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 fixes to bpf_xdp_copy_buf() and bpf_dynptr_copy(), the three
xdp tests fail. With them, all four tests pass.

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

diff --git a/tools/testing/selftests/bpf/prog_tests/dynptr.c b/tools/testing/selftests/bpf/prog_tests/dynptr.c
index 4396560365e8..08382ed076a2 100644
--- a/tools/testing/selftests/bpf/prog_tests/dynptr.c
+++ b/tools/testing/selftests/bpf/prog_tests/dynptr.c
@@ -22,6 +22,10 @@ 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_read_xdp_overlap", SETUP_XDP_PROG},
+	{"test_dynptr_write_xdp_overlap", SETUP_XDP_PROG},
+	{"test_dynptr_skb_load_overlap", SETUP_SKB_PROG_NONLINEAR},
 	{"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..b6c3abb66e4b 100644
--- a/tools/testing/selftests/bpf/progs/dynptr_success.c
+++ b/tools/testing/selftests/bpf/progs/dynptr_success.c
@@ -759,6 +759,134 @@ 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_read_xdp_overlap(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;
+	char *dst;
+
+	bpf_dynptr_from_xdp(xdp, 0, &ptr);
+	if (fill_xdp(&ptr, lin - 10, 84)) {
+		err = 1;
+		return XDP_DROP;
+	}
+	dst = bpf_dynptr_slice_rdwr(&ptr, lin + 10, NULL, 64);
+	if (!dst) {
+		err = 2;
+		return XDP_DROP;
+	}
+	err = bpf_dynptr_read(dst, 64, &ptr, lin - 10, 0);
+	if (err)
+		return XDP_DROP;
+	if (check_xdp(&ptr, lin + 10, 64, lin - 10))
+		err = 3;
+	return XDP_DROP;
+}
+
+SEC("?xdp")
+int test_dynptr_write_xdp_overlap(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;
+	char *src;
+
+	bpf_dynptr_from_xdp(xdp, 0, &ptr);
+	if (fill_xdp(&ptr, lin - 64, 118)) {
+		err = 1;
+		return XDP_DROP;
+	}
+	src = bpf_dynptr_slice_rdwr(&ptr, lin - 64, NULL, 64);
+	if (!src) {
+		err = 2;
+		return XDP_DROP;
+	}
+	err = bpf_dynptr_write(&ptr, lin - 10, src, 64, 0);
+	if (err)
+		return XDP_DROP;
+	if (check_xdp(&ptr, lin - 10, 64, lin - 64))
+		err = 3;
+	return XDP_DROP;
+}
+
+SEC("?tc")
+int test_dynptr_skb_load_overlap(struct __sk_buff *skb)
+{
+	struct bpf_dynptr ptr;
+	char ref[24];
+	char *dst;
+
+	bpf_dynptr_from_skb(skb, 0, &ptr);
+	if (bpf_dynptr_read(ref, 24, &ptr, 20, 0)) {
+		err = 1;
+		return 0;
+	}
+	dst = bpf_dynptr_slice_rdwr(&ptr, 10, NULL, 24);
+	if (!dst) {
+		err = 2;
+		return 0;
+	}
+	if (bpf_dynptr_read(dst, 24, &ptr, 20, 0)) {
+		err = 3;
+		return 0;
+	}
+	if (bpf_memcmp(dst, ref, 24))
+		err = 4;
+	return 0;
+}
+
 char memset_zero_data[] = "data to be zeroed";
 
 SEC("?tp/syscalls/sys_enter_nanosleep")
-- 
2.54.0