[PATCH bpf v2 1/2] bpf, test_run: Fix use-after-free issue in eth_skb_pkt_type()

[PATCH bpf v2 1/2] bpf, test_run: Fix use-after-free issue in eth_skb_pkt_type()

[Shigeru Yoshida]

KMSAN reported a use-after-free issue in eth_skb_pkt_type()[1]. The
cause of the issue was that eth_skb_pkt_type() accessed skb's data
that didn't contain an Ethernet header. This occurs when
bpf_prog_test_run_xdp() passes an invalid value as the user_data
argument to bpf_test_init().

Fix this by returning an error when user_data is less than ETH_HLEN in
bpf_test_init(). Additionally, remove the check for "if (user_size >
size)" as it is unnecessary.

[1]
BUG: KMSAN: use-after-free in eth_skb_pkt_type include/linux/etherdevice.h:627 [inline]
BUG: KMSAN: use-after-free in eth_type_trans+0x4ee/0x980 net/ethernet/eth.c:165
 eth_skb_pkt_type include/linux/etherdevice.h:627 [inline]
 eth_type_trans+0x4ee/0x980 net/ethernet/eth.c:165
 __xdp_build_skb_from_frame+0x5a8/0xa50 net/core/xdp.c:635
 xdp_recv_frames net/bpf/test_run.c:272 [inline]
 xdp_test_run_batch net/bpf/test_run.c:361 [inline]
 bpf_test_run_xdp_live+0x2954/0x3330 net/bpf/test_run.c:390
 bpf_prog_test_run_xdp+0x148e/0x1b10 net/bpf/test_run.c:1318
 bpf_prog_test_run+0x5b7/0xa30 kernel/bpf/syscall.c:4371
 __sys_bpf+0x6a6/0xe20 kernel/bpf/syscall.c:5777
 __do_sys_bpf kernel/bpf/syscall.c:5866 [inline]
 __se_sys_bpf kernel/bpf/syscall.c:5864 [inline]
 __x64_sys_bpf+0xa4/0xf0 kernel/bpf/syscall.c:5864
 x64_sys_call+0x2ea0/0x3d90 arch/x86/include/generated/asm/syscalls_64.h:322
 do_syscall_x64 arch/x86/entry/common.c:52 [inline]
 do_syscall_64+0xd9/0x1d0 arch/x86/entry/common.c:83
 entry_SYSCALL_64_after_hwframe+0x77/0x7f

Uninit was created at:
 free_pages_prepare mm/page_alloc.c:1056 [inline]
 free_unref_page+0x156/0x1320 mm/page_alloc.c:2657
 __free_pages+0xa3/0x1b0 mm/page_alloc.c:4838
 bpf_ringbuf_free kernel/bpf/ringbuf.c:226 [inline]
 ringbuf_map_free+0xff/0x1e0 kernel/bpf/ringbuf.c:235
 bpf_map_free kernel/bpf/syscall.c:838 [inline]
 bpf_map_free_deferred+0x17c/0x310 kernel/bpf/syscall.c:862
 process_one_work kernel/workqueue.c:3229 [inline]
 process_scheduled_works+0xa2b/0x1b60 kernel/workqueue.c:3310
 worker_thread+0xedf/0x1550 kernel/workqueue.c:3391
 kthread+0x535/0x6b0 kernel/kthread.c:389
 ret_from_fork+0x6e/0x90 arch/x86/kernel/process.c:147
 ret_from_fork_asm+0x1a/0x30 arch/x86/entry/entry_64.S:244

CPU: 1 UID: 0 PID: 17276 Comm: syz.1.16450 Not tainted 6.12.0-05490-g9bb88c659673 #8
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.16.3-3.fc41 04/01/2014

Fixes: be3d72a2896c ("bpf: move user_size out of bpf_test_init")
Reported-by: syzkaller <syzkaller@googlegroups.com>
Suggested-by: Martin KaFai Lau <martin.lau@linux.dev>
Signed-off-by: Shigeru Yoshida <syoshida@redhat.com>
---
v2:
- Rewrote the code as suggested by Martin.
- Fixed the broken tests.
v1:
- https://lore.kernel.org/all/20241201152735.106681-1-syoshida@redhat.com/
---
 net/bpf/test_run.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/net/bpf/test_run.c b/net/bpf/test_run.c
index 501ec4249fed..8612023bec60 100644
--- a/net/bpf/test_run.c
+++ b/net/bpf/test_run.c
@@ -660,12 +660,9 @@ static void *bpf_test_init(const union bpf_attr *kattr, u32 user_size,
 	void __user *data_in = u64_to_user_ptr(kattr->test.data_in);
 	void *data;
 
-	if (size < ETH_HLEN || size > PAGE_SIZE - headroom - tailroom)
+	if (user_size < ETH_HLEN || user_size > PAGE_SIZE - headroom - tailroom)
 		return ERR_PTR(-EINVAL);
 
-	if (user_size > size)
-		return ERR_PTR(-EMSGSIZE);
-
 	size = SKB_DATA_ALIGN(size);
 	data = kzalloc(size + headroom + tailroom, GFP_USER);
 	if (!data)
-- 
2.48.1

[PATCH bpf v2 2/2] selftests/bpf: Adjust data size to have ETH_HLEN

[Shigeru Yoshida]

The function bpf_test_init() now returns an error if user_size
(.data_size_in) is less than ETH_HLEN, causing the tests to
fail. Adjust the data size to ensure it meets the requirement of
ETH_HLEN.

Signed-off-by: Shigeru Yoshida <syoshida@redhat.com>
---
 .../testing/selftests/bpf/prog_tests/xdp_cpumap_attach.c  | 4 ++--
 .../testing/selftests/bpf/prog_tests/xdp_devmap_attach.c  | 8 ++++----
 2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/tools/testing/selftests/bpf/prog_tests/xdp_cpumap_attach.c b/tools/testing/selftests/bpf/prog_tests/xdp_cpumap_attach.c
index c7f74f068e78..df27535995af 100644
--- a/tools/testing/selftests/bpf/prog_tests/xdp_cpumap_attach.c
+++ b/tools/testing/selftests/bpf/prog_tests/xdp_cpumap_attach.c
@@ -52,10 +52,10 @@ static void test_xdp_with_cpumap_helpers(void)
 	ASSERT_EQ(info.id, val.bpf_prog.id, "Match program id to cpumap entry prog_id");
 
 	/* send a packet to trigger any potential bugs in there */
-	char data[10] = {};
+	char data[ETH_HLEN] = {};
 	DECLARE_LIBBPF_OPTS(bpf_test_run_opts, opts,
 			    .data_in = &data,
-			    .data_size_in = 10,
+			    .data_size_in = sizeof(data),
 			    .flags = BPF_F_TEST_XDP_LIVE_FRAMES,
 			    .repeat = 1,
 		);
diff --git a/tools/testing/selftests/bpf/prog_tests/xdp_devmap_attach.c b/tools/testing/selftests/bpf/prog_tests/xdp_devmap_attach.c
index 27ffed17d4be..461ab18705d5 100644
--- a/tools/testing/selftests/bpf/prog_tests/xdp_devmap_attach.c
+++ b/tools/testing/selftests/bpf/prog_tests/xdp_devmap_attach.c
@@ -23,7 +23,7 @@ static void test_xdp_with_devmap_helpers(void)
 	__u32 len = sizeof(info);
 	int err, dm_fd, dm_fd_redir, map_fd;
 	struct nstoken *nstoken = NULL;
-	char data[10] = {};
+	char data[ETH_HLEN] = {};
 	__u32 idx = 0;
 
 	SYS(out_close, "ip netns add %s", TEST_NS);
@@ -58,7 +58,7 @@ static void test_xdp_with_devmap_helpers(void)
 	/* send a packet to trigger any potential bugs in there */
 	DECLARE_LIBBPF_OPTS(bpf_test_run_opts, opts,
 			    .data_in = &data,
-			    .data_size_in = 10,
+			    .data_size_in = sizeof(data),
 			    .flags = BPF_F_TEST_XDP_LIVE_FRAMES,
 			    .repeat = 1,
 		);
@@ -158,7 +158,7 @@ static void test_xdp_with_devmap_helpers_veth(void)
 	struct nstoken *nstoken = NULL;
 	__u32 len = sizeof(info);
 	int err, dm_fd, dm_fd_redir, map_fd, ifindex_dst;
-	char data[10] = {};
+	char data[ETH_HLEN] = {};
 	__u32 idx = 0;
 
 	SYS(out_close, "ip netns add %s", TEST_NS);
@@ -208,7 +208,7 @@ static void test_xdp_with_devmap_helpers_veth(void)
 	/* send a packet to trigger any potential bugs in there */
 	DECLARE_LIBBPF_OPTS(bpf_test_run_opts, opts,
 			    .data_in = &data,
-			    .data_size_in = 10,
+			    .data_size_in = sizeof(data),
 			    .flags = BPF_F_TEST_XDP_LIVE_FRAMES,
 			    .repeat = 1,
 		);
-- 
2.48.1

Re: [PATCH bpf v2 2/2] selftests/bpf: Adjust data size to have ETH_HLEN

[Stanislav Fomichev]

On 01/22, Shigeru Yoshida wrote:
> The function bpf_test_init() now returns an error if user_size
> (.data_size_in) is less than ETH_HLEN, causing the tests to
> fail. Adjust the data size to ensure it meets the requirement of
> ETH_HLEN.
> 
> Signed-off-by: Shigeru Yoshida <syoshida@redhat.com>
> ---
>  .../testing/selftests/bpf/prog_tests/xdp_cpumap_attach.c  | 4 ++--
>  .../testing/selftests/bpf/prog_tests/xdp_devmap_attach.c  | 8 ++++----
>  2 files changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/tools/testing/selftests/bpf/prog_tests/xdp_cpumap_attach.c b/tools/testing/selftests/bpf/prog_tests/xdp_cpumap_attach.c
> index c7f74f068e78..df27535995af 100644
> --- a/tools/testing/selftests/bpf/prog_tests/xdp_cpumap_attach.c
> +++ b/tools/testing/selftests/bpf/prog_tests/xdp_cpumap_attach.c
> @@ -52,10 +52,10 @@ static void test_xdp_with_cpumap_helpers(void)
>  	ASSERT_EQ(info.id, val.bpf_prog.id, "Match program id to cpumap entry prog_id");
>  
>  	/* send a packet to trigger any potential bugs in there */
> -	char data[10] = {};
> +	char data[ETH_HLEN] = {};
>  	DECLARE_LIBBPF_OPTS(bpf_test_run_opts, opts,
>  			    .data_in = &data,
> -			    .data_size_in = 10,
> +			    .data_size_in = sizeof(data),
>  			    .flags = BPF_F_TEST_XDP_LIVE_FRAMES,
>  			    .repeat = 1,
>  		);

We should still keep 10, but change the ASSERT_OK below to expect the
error instead. Looking at the comment above, the purpose of the test
is to exercise that error case.

Same for the other two cases.

Re: [PATCH bpf v2 1/2] bpf, test_run: Fix use-after-free issue in eth_skb_pkt_type()

[Stanislav Fomichev]

On 01/22, Shigeru Yoshida wrote:
> KMSAN reported a use-after-free issue in eth_skb_pkt_type()[1]. The
> cause of the issue was that eth_skb_pkt_type() accessed skb's data
> that didn't contain an Ethernet header. This occurs when
> bpf_prog_test_run_xdp() passes an invalid value as the user_data
> argument to bpf_test_init().
> 
> Fix this by returning an error when user_data is less than ETH_HLEN in
> bpf_test_init(). Additionally, remove the check for "if (user_size >
> size)" as it is unnecessary.
> 
> [1]
> BUG: KMSAN: use-after-free in eth_skb_pkt_type include/linux/etherdevice.h:627 [inline]
> BUG: KMSAN: use-after-free in eth_type_trans+0x4ee/0x980 net/ethernet/eth.c:165
>  eth_skb_pkt_type include/linux/etherdevice.h:627 [inline]
>  eth_type_trans+0x4ee/0x980 net/ethernet/eth.c:165
>  __xdp_build_skb_from_frame+0x5a8/0xa50 net/core/xdp.c:635
>  xdp_recv_frames net/bpf/test_run.c:272 [inline]
>  xdp_test_run_batch net/bpf/test_run.c:361 [inline]
>  bpf_test_run_xdp_live+0x2954/0x3330 net/bpf/test_run.c:390
>  bpf_prog_test_run_xdp+0x148e/0x1b10 net/bpf/test_run.c:1318
>  bpf_prog_test_run+0x5b7/0xa30 kernel/bpf/syscall.c:4371
>  __sys_bpf+0x6a6/0xe20 kernel/bpf/syscall.c:5777
>  __do_sys_bpf kernel/bpf/syscall.c:5866 [inline]
>  __se_sys_bpf kernel/bpf/syscall.c:5864 [inline]
>  __x64_sys_bpf+0xa4/0xf0 kernel/bpf/syscall.c:5864
>  x64_sys_call+0x2ea0/0x3d90 arch/x86/include/generated/asm/syscalls_64.h:322
>  do_syscall_x64 arch/x86/entry/common.c:52 [inline]
>  do_syscall_64+0xd9/0x1d0 arch/x86/entry/common.c:83
>  entry_SYSCALL_64_after_hwframe+0x77/0x7f
> 
> Uninit was created at:
>  free_pages_prepare mm/page_alloc.c:1056 [inline]
>  free_unref_page+0x156/0x1320 mm/page_alloc.c:2657
>  __free_pages+0xa3/0x1b0 mm/page_alloc.c:4838
>  bpf_ringbuf_free kernel/bpf/ringbuf.c:226 [inline]
>  ringbuf_map_free+0xff/0x1e0 kernel/bpf/ringbuf.c:235
>  bpf_map_free kernel/bpf/syscall.c:838 [inline]
>  bpf_map_free_deferred+0x17c/0x310 kernel/bpf/syscall.c:862
>  process_one_work kernel/workqueue.c:3229 [inline]
>  process_scheduled_works+0xa2b/0x1b60 kernel/workqueue.c:3310
>  worker_thread+0xedf/0x1550 kernel/workqueue.c:3391
>  kthread+0x535/0x6b0 kernel/kthread.c:389
>  ret_from_fork+0x6e/0x90 arch/x86/kernel/process.c:147
>  ret_from_fork_asm+0x1a/0x30 arch/x86/entry/entry_64.S:244
> 
> CPU: 1 UID: 0 PID: 17276 Comm: syz.1.16450 Not tainted 6.12.0-05490-g9bb88c659673 #8
> Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.16.3-3.fc41 04/01/2014
> 
> Fixes: be3d72a2896c ("bpf: move user_size out of bpf_test_init")
> Reported-by: syzkaller <syzkaller@googlegroups.com>
> Suggested-by: Martin KaFai Lau <martin.lau@linux.dev>
> Signed-off-by: Shigeru Yoshida <syoshida@redhat.com>

Acked-by: Stanislav Fomichev <sdf@fomichev.me>

Re: [PATCH bpf v2 1/2] bpf, test_run: Fix use-after-free issue in eth_skb_pkt_type()

[Daniel Borkmann]

On 1/21/25 4:06 PM, Shigeru Yoshida wrote:
> KMSAN reported a use-after-free issue in eth_skb_pkt_type()[1]. The
> cause of the issue was that eth_skb_pkt_type() accessed skb's data
> that didn't contain an Ethernet header. This occurs when
> bpf_prog_test_run_xdp() passes an invalid value as the user_data
> argument to bpf_test_init().
> 
> Fix this by returning an error when user_data is less than ETH_HLEN in
> bpf_test_init(). Additionally, remove the check for "if (user_size >
> size)" as it is unnecessary.
> 
> [1]
> BUG: KMSAN: use-after-free in eth_skb_pkt_type include/linux/etherdevice.h:627 [inline]
> BUG: KMSAN: use-after-free in eth_type_trans+0x4ee/0x980 net/ethernet/eth.c:165
>   eth_skb_pkt_type include/linux/etherdevice.h:627 [inline]
>   eth_type_trans+0x4ee/0x980 net/ethernet/eth.c:165
>   __xdp_build_skb_from_frame+0x5a8/0xa50 net/core/xdp.c:635
>   xdp_recv_frames net/bpf/test_run.c:272 [inline]
>   xdp_test_run_batch net/bpf/test_run.c:361 [inline]
>   bpf_test_run_xdp_live+0x2954/0x3330 net/bpf/test_run.c:390
>   bpf_prog_test_run_xdp+0x148e/0x1b10 net/bpf/test_run.c:1318
>   bpf_prog_test_run+0x5b7/0xa30 kernel/bpf/syscall.c:4371
>   __sys_bpf+0x6a6/0xe20 kernel/bpf/syscall.c:5777
>   __do_sys_bpf kernel/bpf/syscall.c:5866 [inline]
>   __se_sys_bpf kernel/bpf/syscall.c:5864 [inline]
>   __x64_sys_bpf+0xa4/0xf0 kernel/bpf/syscall.c:5864
>   x64_sys_call+0x2ea0/0x3d90 arch/x86/include/generated/asm/syscalls_64.h:322
>   do_syscall_x64 arch/x86/entry/common.c:52 [inline]
>   do_syscall_64+0xd9/0x1d0 arch/x86/entry/common.c:83
>   entry_SYSCALL_64_after_hwframe+0x77/0x7f
> 
> Uninit was created at:
>   free_pages_prepare mm/page_alloc.c:1056 [inline]
>   free_unref_page+0x156/0x1320 mm/page_alloc.c:2657
>   __free_pages+0xa3/0x1b0 mm/page_alloc.c:4838
>   bpf_ringbuf_free kernel/bpf/ringbuf.c:226 [inline]
>   ringbuf_map_free+0xff/0x1e0 kernel/bpf/ringbuf.c:235
>   bpf_map_free kernel/bpf/syscall.c:838 [inline]
>   bpf_map_free_deferred+0x17c/0x310 kernel/bpf/syscall.c:862
>   process_one_work kernel/workqueue.c:3229 [inline]
>   process_scheduled_works+0xa2b/0x1b60 kernel/workqueue.c:3310
>   worker_thread+0xedf/0x1550 kernel/workqueue.c:3391
>   kthread+0x535/0x6b0 kernel/kthread.c:389
>   ret_from_fork+0x6e/0x90 arch/x86/kernel/process.c:147
>   ret_from_fork_asm+0x1a/0x30 arch/x86/entry/entry_64.S:244
> 
> CPU: 1 UID: 0 PID: 17276 Comm: syz.1.16450 Not tainted 6.12.0-05490-g9bb88c659673 #8
> Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.16.3-3.fc41 04/01/2014
> 
> Fixes: be3d72a2896c ("bpf: move user_size out of bpf_test_init")
> Reported-by: syzkaller <syzkaller@googlegroups.com>
> Suggested-by: Martin KaFai Lau <martin.lau@linux.dev>
> Signed-off-by: Shigeru Yoshida <syoshida@redhat.com>

Acked-by: Daniel Borkmann <daniel@iogearbox.net>

Re: [PATCH bpf v2 2/2] selftests/bpf: Adjust data size to have ETH_HLEN

[Martin KaFai Lau]

On 1/23/25 11:18 AM, Stanislav Fomichev wrote:
> On 01/22, Shigeru Yoshida wrote:
>> The function bpf_test_init() now returns an error if user_size
>> (.data_size_in) is less than ETH_HLEN, causing the tests to
>> fail. Adjust the data size to ensure it meets the requirement of
>> ETH_HLEN.
>>
>> Signed-off-by: Shigeru Yoshida <syoshida@redhat.com>
>> ---
>>   .../testing/selftests/bpf/prog_tests/xdp_cpumap_attach.c  | 4 ++--
>>   .../testing/selftests/bpf/prog_tests/xdp_devmap_attach.c  | 8 ++++----
>>   2 files changed, 6 insertions(+), 6 deletions(-)
>>
>> diff --git a/tools/testing/selftests/bpf/prog_tests/xdp_cpumap_attach.c b/tools/testing/selftests/bpf/prog_tests/xdp_cpumap_attach.c
>> index c7f74f068e78..df27535995af 100644
>> --- a/tools/testing/selftests/bpf/prog_tests/xdp_cpumap_attach.c
>> +++ b/tools/testing/selftests/bpf/prog_tests/xdp_cpumap_attach.c
>> @@ -52,10 +52,10 @@ static void test_xdp_with_cpumap_helpers(void)
>>   	ASSERT_EQ(info.id, val.bpf_prog.id, "Match program id to cpumap entry prog_id");
>>   
>>   	/* send a packet to trigger any potential bugs in there */
>> -	char data[10] = {};
>> +	char data[ETH_HLEN] = {};
>>   	DECLARE_LIBBPF_OPTS(bpf_test_run_opts, opts,
>>   			    .data_in = &data,
>> -			    .data_size_in = 10,
>> +			    .data_size_in = sizeof(data),
>>   			    .flags = BPF_F_TEST_XDP_LIVE_FRAMES,
>>   			    .repeat = 1,
>>   		);
> 
> We should still keep 10, but change the ASSERT_OK below to expect the
> error instead. Looking at the comment above, the purpose of the test
> is to exercise that error case.
> 

I think the bpf_prog_test_run_opts in this dev/cpumap test is to check the 
bpf_redirect_map() helper, so it expects the bpf_prog_test_run_opts to succeed.

It just happens the current data[10] cannot trigger the fixed bug because the 
bpf prog returns a XDP_REDIRECT instead of XDP_PASS, so xdp_recv_frames is not 
called.

To test patch 1, a separate test is probably needed to trigger the bug in 
xdp_recv_frames() with a bpf prog returning XDP_PASS.

Re: [PATCH bpf v2 2/2] selftests/bpf: Adjust data size to have ETH_HLEN

[Stanislav Fomichev]

On 01/23, Martin KaFai Lau wrote:
> On 1/23/25 11:18 AM, Stanislav Fomichev wrote:
> > On 01/22, Shigeru Yoshida wrote:
> > > The function bpf_test_init() now returns an error if user_size
> > > (.data_size_in) is less than ETH_HLEN, causing the tests to
> > > fail. Adjust the data size to ensure it meets the requirement of
> > > ETH_HLEN.
> > > 
> > > Signed-off-by: Shigeru Yoshida <syoshida@redhat.com>
> > > ---
> > >   .../testing/selftests/bpf/prog_tests/xdp_cpumap_attach.c  | 4 ++--
> > >   .../testing/selftests/bpf/prog_tests/xdp_devmap_attach.c  | 8 ++++----
> > >   2 files changed, 6 insertions(+), 6 deletions(-)
> > > 
> > > diff --git a/tools/testing/selftests/bpf/prog_tests/xdp_cpumap_attach.c b/tools/testing/selftests/bpf/prog_tests/xdp_cpumap_attach.c
> > > index c7f74f068e78..df27535995af 100644
> > > --- a/tools/testing/selftests/bpf/prog_tests/xdp_cpumap_attach.c
> > > +++ b/tools/testing/selftests/bpf/prog_tests/xdp_cpumap_attach.c
> > > @@ -52,10 +52,10 @@ static void test_xdp_with_cpumap_helpers(void)
> > >   	ASSERT_EQ(info.id, val.bpf_prog.id, "Match program id to cpumap entry prog_id");
> > >   	/* send a packet to trigger any potential bugs in there */
> > > -	char data[10] = {};
> > > +	char data[ETH_HLEN] = {};
> > >   	DECLARE_LIBBPF_OPTS(bpf_test_run_opts, opts,
> > >   			    .data_in = &data,
> > > -			    .data_size_in = 10,
> > > +			    .data_size_in = sizeof(data),
> > >   			    .flags = BPF_F_TEST_XDP_LIVE_FRAMES,
> > >   			    .repeat = 1,
> > >   		);
> > 
> > We should still keep 10, but change the ASSERT_OK below to expect the
> > error instead. Looking at the comment above, the purpose of the test
> > is to exercise that error case.
> > 
> 
> I think the bpf_prog_test_run_opts in this dev/cpumap test is to check the
> bpf_redirect_map() helper, so it expects the bpf_prog_test_run_opts to
> succeed.
> 
> It just happens the current data[10] cannot trigger the fixed bug because
> the bpf prog returns a XDP_REDIRECT instead of XDP_PASS, so xdp_recv_frames
> is not called.
> 
> To test patch 1, a separate test is probably needed to trigger the bug in
> xdp_recv_frames() with a bpf prog returning XDP_PASS.

Ah, yes, you're right, I missed the remaining parts that make sure
the redirect happens :-(

Re: [PATCH bpf v2 1/2] bpf, test_run: Fix use-after-free issue in eth_skb_pkt_type()

[patchwork-bot+netdevbpf]

Hello:

This series was applied to bpf/bpf.git (master)
by Martin KaFai Lau <martin.lau@kernel.org>:

On Wed, 22 Jan 2025 00:06:42 +0900 you wrote:
> KMSAN reported a use-after-free issue in eth_skb_pkt_type()[1]. The
> cause of the issue was that eth_skb_pkt_type() accessed skb's data
> that didn't contain an Ethernet header. This occurs when
> bpf_prog_test_run_xdp() passes an invalid value as the user_data
> argument to bpf_test_init().
> 
> Fix this by returning an error when user_data is less than ETH_HLEN in
> bpf_test_init(). Additionally, remove the check for "if (user_size >
> size)" as it is unnecessary.
> 
> [...]

Here is the summary with links:
  - [bpf,v2,1/2] bpf, test_run: Fix use-after-free issue in eth_skb_pkt_type()
    https://git.kernel.org/bpf/bpf/c/c5e8e573d6e4
  - [bpf,v2,2/2] selftests/bpf: Adjust data size to have ETH_HLEN
    https://git.kernel.org/bpf/bpf/c/f9f03a0a6d2d

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html

Re: [PATCH bpf v2 2/2] selftests/bpf: Adjust data size to have ETH_HLEN

[Martin KaFai Lau]

On 1/23/25 6:45 PM, Stanislav Fomichev wrote:
> On 01/23, Martin KaFai Lau wrote:
>> On 1/23/25 11:18 AM, Stanislav Fomichev wrote:
>>> On 01/22, Shigeru Yoshida wrote:
>>>> The function bpf_test_init() now returns an error if user_size
>>>> (.data_size_in) is less than ETH_HLEN, causing the tests to
>>>> fail. Adjust the data size to ensure it meets the requirement of
>>>> ETH_HLEN.
>>>>
>>>> Signed-off-by: Shigeru Yoshida <syoshida@redhat.com>
>>>> ---
>>>>    .../testing/selftests/bpf/prog_tests/xdp_cpumap_attach.c  | 4 ++--
>>>>    .../testing/selftests/bpf/prog_tests/xdp_devmap_attach.c  | 8 ++++----
>>>>    2 files changed, 6 insertions(+), 6 deletions(-)
>>>>
>>>> diff --git a/tools/testing/selftests/bpf/prog_tests/xdp_cpumap_attach.c b/tools/testing/selftests/bpf/prog_tests/xdp_cpumap_attach.c
>>>> index c7f74f068e78..df27535995af 100644
>>>> --- a/tools/testing/selftests/bpf/prog_tests/xdp_cpumap_attach.c
>>>> +++ b/tools/testing/selftests/bpf/prog_tests/xdp_cpumap_attach.c
>>>> @@ -52,10 +52,10 @@ static void test_xdp_with_cpumap_helpers(void)
>>>>    	ASSERT_EQ(info.id, val.bpf_prog.id, "Match program id to cpumap entry prog_id");
>>>>    	/* send a packet to trigger any potential bugs in there */
>>>> -	char data[10] = {};
>>>> +	char data[ETH_HLEN] = {};
>>>>    	DECLARE_LIBBPF_OPTS(bpf_test_run_opts, opts,
>>>>    			    .data_in = &data,
>>>> -			    .data_size_in = 10,
>>>> +			    .data_size_in = sizeof(data),
>>>>    			    .flags = BPF_F_TEST_XDP_LIVE_FRAMES,
>>>>    			    .repeat = 1,
>>>>    		);
>>>
>>> We should still keep 10, but change the ASSERT_OK below to expect the
>>> error instead. Looking at the comment above, the purpose of the test
>>> is to exercise that error case.
>>>
>>
>> I think the bpf_prog_test_run_opts in this dev/cpumap test is to check the
>> bpf_redirect_map() helper, so it expects the bpf_prog_test_run_opts to
>> succeed.
>>
>> It just happens the current data[10] cannot trigger the fixed bug because
>> the bpf prog returns a XDP_REDIRECT instead of XDP_PASS, so xdp_recv_frames
>> is not called.
>>
>> To test patch 1, a separate test is probably needed to trigger the bug in
>> xdp_recv_frames() with a bpf prog returning XDP_PASS.
> 
> Ah, yes, you're right, I missed the remaining parts that make sure
> the redirect happens

Thanks for confirming and the review.

Applied the fix.

Shigeru, please followup with a selftest to test the "less than ETH_HLEN" bug 
addressed in Patch 1. You can reuse some of the boilerplate codes from the 
xdp_cpumap_attach.c. The bpf prog can simply be "return XDP_PASS;" and ensure 
that BPF_F_TEST_XDP_LIVE_FRAMES is included in the bpf_test_run_opts. Thanks.

Re: [PATCH bpf v2 2/2] selftests/bpf: Adjust data size to have ETH_HLEN

[Shigeru Yoshida]

On Fri, 24 Jan 2025 11:22:16 -0800, Martin KaFai Lau wrote:
> On 1/23/25 6:45 PM, Stanislav Fomichev wrote:
>> On 01/23, Martin KaFai Lau wrote:
>>> On 1/23/25 11:18 AM, Stanislav Fomichev wrote:
>>>> On 01/22, Shigeru Yoshida wrote:
>>>>> The function bpf_test_init() now returns an error if user_size
>>>>> (.data_size_in) is less than ETH_HLEN, causing the tests to
>>>>> fail. Adjust the data size to ensure it meets the requirement of
>>>>> ETH_HLEN.
>>>>>
>>>>> Signed-off-by: Shigeru Yoshida <syoshida@redhat.com>
>>>>> ---
>>>>>    .../testing/selftests/bpf/prog_tests/xdp_cpumap_attach.c  | 4 ++--
>>>>>    .../testing/selftests/bpf/prog_tests/xdp_devmap_attach.c  | 8 ++++----
>>>>>    2 files changed, 6 insertions(+), 6 deletions(-)
>>>>>
>>>>> diff --git
>>>>> a/tools/testing/selftests/bpf/prog_tests/xdp_cpumap_attach.c
>>>>> b/tools/testing/selftests/bpf/prog_tests/xdp_cpumap_attach.c
>>>>> index c7f74f068e78..df27535995af 100644
>>>>> --- a/tools/testing/selftests/bpf/prog_tests/xdp_cpumap_attach.c
>>>>> +++ b/tools/testing/selftests/bpf/prog_tests/xdp_cpumap_attach.c
>>>>> @@ -52,10 +52,10 @@ static void test_xdp_with_cpumap_helpers(void)
>>>>>    	ASSERT_EQ(info.id, val.bpf_prog.id, "Match program id to cpumap entry
>>>>>    	prog_id");
>>>>>    	/* send a packet to trigger any potential bugs in there */
>>>>> -	char data[10] = {};
>>>>> +	char data[ETH_HLEN] = {};
>>>>>    	DECLARE_LIBBPF_OPTS(bpf_test_run_opts, opts,
>>>>>    			    .data_in = &data,
>>>>> -			    .data_size_in = 10,
>>>>> +			    .data_size_in = sizeof(data),
>>>>>    			    .flags = BPF_F_TEST_XDP_LIVE_FRAMES,
>>>>>    			    .repeat = 1,
>>>>>    		);
>>>>
>>>> We should still keep 10, but change the ASSERT_OK below to expect the
>>>> error instead. Looking at the comment above, the purpose of the test
>>>> is to exercise that error case.
>>>>
>>>
>>> I think the bpf_prog_test_run_opts in this dev/cpumap test is to check
>>> the
>>> bpf_redirect_map() helper, so it expects the bpf_prog_test_run_opts to
>>> succeed.
>>>
>>> It just happens the current data[10] cannot trigger the fixed bug
>>> because
>>> the bpf prog returns a XDP_REDIRECT instead of XDP_PASS, so
>>> xdp_recv_frames
>>> is not called.
>>>
>>> To test patch 1, a separate test is probably needed to trigger the bug
>>> in
>>> xdp_recv_frames() with a bpf prog returning XDP_PASS.
>> Ah, yes, you're right, I missed the remaining parts that make sure
>> the redirect happens
> 
> Thanks for confirming and the review.
> 
> Applied the fix.

Hi Martin, Stanislav,

Thank you for your comments and feedback!

> Shigeru, please followup with a selftest to test the "less than
> ETH_HLEN" bug addressed in Patch 1. You can reuse some of the
> boilerplate codes from the xdp_cpumap_attach.c. The bpf prog can
> simply be "return XDP_PASS;" and ensure that
> BPF_F_TEST_XDP_LIVE_FRAMES is included in the
> bpf_test_run_opts. Thanks.

I'm not very familiar with bpf and its selftests, but I will try to
make a new test according to your advice.

Thanks,
Shigeru