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

[PATCH bpf] 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().

[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>
Signed-off-by: Shigeru Yoshida <syoshida@redhat.com>
---
 net/bpf/test_run.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/bpf/test_run.c b/net/bpf/test_run.c
index 501ec4249fed..756250aa890f 100644
--- a/net/bpf/test_run.c
+++ b/net/bpf/test_run.c
@@ -663,7 +663,7 @@ static void *bpf_test_init(const union bpf_attr *kattr, u32 user_size,
 	if (size < ETH_HLEN || size > PAGE_SIZE - headroom - tailroom)
 		return ERR_PTR(-EINVAL);
 
-	if (user_size > size)
+	if (user_size < ETH_HLEN || user_size > size)
 		return ERR_PTR(-EMSGSIZE);
 
 	size = SKB_DATA_ALIGN(size);
-- 
2.47.0

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

[Alexander Potapenko]

On Sun, Dec 1, 2024 at 4:27 PM Shigeru Yoshida <syoshida@redhat.com> 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().

Hi Shigeru,

Thanks for taking care of this!

Am I understanding right that this bug was reported by your own
syzkaller instance?

Otherwise, if the report originates from syzkaller.appspot.com, could
you please append the bug hash to the Reported-by: tag?

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

[Shigeru Yoshida]

On Mon, 2 Dec 2024 10:32:04 +0100, Alexander Potapenko wrote:
> On Sun, Dec 1, 2024 at 4:27 PM Shigeru Yoshida <syoshida@redhat.com> 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().
> 
> Hi Shigeru,
> 
> Thanks for taking care of this!
> 
> Am I understanding right that this bug was reported by your own
> syzkaller instance?

Hi Alexander,

Thank you for your comment!

Yes, this issue was identified by running my own syzkaller locally.

I added the "Reported-by: syzkaller <syzkaller@googlegroups.com>" tag
to give credit to the syzkaller community.

> Otherwise, if the report originates from syzkaller.appspot.com, could
> you please append the bug hash to the Reported-by: tag?

I checked syzkaller.appspot.com, but this issue does not seem to be
reported.

Thanks,  
Shigeru

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

[Stanislav Fomichev]

On 12/02, 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().
> 
> [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>
> Signed-off-by: Shigeru Yoshida <syoshida@redhat.com>
> ---
>  net/bpf/test_run.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/net/bpf/test_run.c b/net/bpf/test_run.c
> index 501ec4249fed..756250aa890f 100644
> --- a/net/bpf/test_run.c
> +++ b/net/bpf/test_run.c
> @@ -663,7 +663,7 @@ static void *bpf_test_init(const union bpf_attr *kattr, u32 user_size,
>  	if (size < ETH_HLEN || size > PAGE_SIZE - headroom - tailroom)
>  		return ERR_PTR(-EINVAL);
>  
> -	if (user_size > size)
> +	if (user_size < ETH_HLEN || user_size > size)
>  		return ERR_PTR(-EMSGSIZE);
>  
>  	size = SKB_DATA_ALIGN(size);
> -- 
> 2.47.0
> 

I wonder whether 'size < ETH_HLEN' above is needed after your patch.
Feels like 'user_size < ETH_HLEN' supersedes it.

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

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

[Martin KaFai Lau]

On 12/2/24 8:15 AM, Stanislav Fomichev wrote:
> On 12/02, 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().
>>
>> [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>
>> Signed-off-by: Shigeru Yoshida <syoshida@redhat.com>
>> ---
>>   net/bpf/test_run.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/net/bpf/test_run.c b/net/bpf/test_run.c
>> index 501ec4249fed..756250aa890f 100644
>> --- a/net/bpf/test_run.c
>> +++ b/net/bpf/test_run.c
>> @@ -663,7 +663,7 @@ static void *bpf_test_init(const union bpf_attr *kattr, u32 user_size,
>>   	if (size < ETH_HLEN || size > PAGE_SIZE - headroom - tailroom)
>>   		return ERR_PTR(-EINVAL);
>>   
>> -	if (user_size > size)
>> +	if (user_size < ETH_HLEN || user_size > size)
>>   		return ERR_PTR(-EMSGSIZE);
>>   
>>   	size = SKB_DATA_ALIGN(size);
>> -- 
>> 2.47.0
>>
> 
> I wonder whether 'size < ETH_HLEN' above is needed after your patch.
> Feels like 'user_size < ETH_HLEN' supersedes it.

May be fixing it by replacing the existing "size" check with "user_size" check? 
Seems more intuitive that checking is needed on the "user_"size instead of the 
"size". The "if (user_size > size)" check looks useless also. Something like this?

-	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);
-

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

[Martin KaFai Lau]

On 12/2/24 1:38 PM, Martin KaFai Lau wrote:
> On 12/2/24 8:15 AM, Stanislav Fomichev wrote:
>> On 12/02, 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().
>>>
>>> [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>
>>> Signed-off-by: Shigeru Yoshida <syoshida@redhat.com>
>>> ---
>>>   net/bpf/test_run.c | 2 +-
>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/net/bpf/test_run.c b/net/bpf/test_run.c
>>> index 501ec4249fed..756250aa890f 100644
>>> --- a/net/bpf/test_run.c
>>> +++ b/net/bpf/test_run.c
>>> @@ -663,7 +663,7 @@ static void *bpf_test_init(const union bpf_attr *kattr, 
>>> u32 user_size,
>>>       if (size < ETH_HLEN || size > PAGE_SIZE - headroom - tailroom)
>>>           return ERR_PTR(-EINVAL);
>>> -    if (user_size > size)
>>> +    if (user_size < ETH_HLEN || user_size > size)
>>>           return ERR_PTR(-EMSGSIZE);
>>>       size = SKB_DATA_ALIGN(size);
>>> -- 
>>> 2.47.0
>>>
>>
>> I wonder whether 'size < ETH_HLEN' above is needed after your patch.
>> Feels like 'user_size < ETH_HLEN' supersedes it.
> 
> May be fixing it by replacing the existing "size" check with "user_size" check? 
> Seems more intuitive that checking is needed on the "user_"size instead of the 
> "size". The "if (user_size > size)" check looks useless also. Something like this?
> 
> -    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);
> -

just noticed the xdp_cpumap_attach test failed also. The test needs an 
adjustment in the .data_in and .data_size_in to have at least ETH_HLEN.

pw-bot: cr

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

[Stanislav Fomichev]

On 12/02, Martin KaFai Lau wrote:
> On 12/2/24 8:15 AM, Stanislav Fomichev wrote:
> > On 12/02, 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().
> > > 
> > > [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>
> > > Signed-off-by: Shigeru Yoshida <syoshida@redhat.com>
> > > ---
> > >   net/bpf/test_run.c | 2 +-
> > >   1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/net/bpf/test_run.c b/net/bpf/test_run.c
> > > index 501ec4249fed..756250aa890f 100644
> > > --- a/net/bpf/test_run.c
> > > +++ b/net/bpf/test_run.c
> > > @@ -663,7 +663,7 @@ static void *bpf_test_init(const union bpf_attr *kattr, u32 user_size,
> > >   	if (size < ETH_HLEN || size > PAGE_SIZE - headroom - tailroom)
> > >   		return ERR_PTR(-EINVAL);
> > > -	if (user_size > size)
> > > +	if (user_size < ETH_HLEN || user_size > size)
> > >   		return ERR_PTR(-EMSGSIZE);
> > >   	size = SKB_DATA_ALIGN(size);
> > > -- 
> > > 2.47.0
> > > 
> > 
> > I wonder whether 'size < ETH_HLEN' above is needed after your patch.
> > Feels like 'user_size < ETH_HLEN' supersedes it.
> 
> May be fixing it by replacing the existing "size" check with "user_size"
> check? Seems more intuitive that checking is needed on the "user_"size
> instead of the "size". The "if (user_size > size)" check looks useless also.
> Something like this?
> 
> -	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);
> -
> 

Agreed, that should do it. IIUC, 'user_size > size' only makes sense
for the bpf_prog_test_run_xdp case and the caller handles this case
anyway (size > max_data_sz).

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

[Shigeru Yoshida]

On Mon, 2 Dec 2024 16:06:55 -0800, Stanislav Fomichev wrote:
> On 12/02, Martin KaFai Lau wrote:
>> On 12/2/24 8:15 AM, Stanislav Fomichev wrote:
>> > On 12/02, 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().
>> > > 
>> > > [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>
>> > > Signed-off-by: Shigeru Yoshida <syoshida@redhat.com>
>> > > ---
>> > >   net/bpf/test_run.c | 2 +-
>> > >   1 file changed, 1 insertion(+), 1 deletion(-)
>> > > 
>> > > diff --git a/net/bpf/test_run.c b/net/bpf/test_run.c
>> > > index 501ec4249fed..756250aa890f 100644
>> > > --- a/net/bpf/test_run.c
>> > > +++ b/net/bpf/test_run.c
>> > > @@ -663,7 +663,7 @@ static void *bpf_test_init(const union bpf_attr *kattr, u32 user_size,
>> > >   	if (size < ETH_HLEN || size > PAGE_SIZE - headroom - tailroom)
>> > >   		return ERR_PTR(-EINVAL);
>> > > -	if (user_size > size)
>> > > +	if (user_size < ETH_HLEN || user_size > size)
>> > >   		return ERR_PTR(-EMSGSIZE);
>> > >   	size = SKB_DATA_ALIGN(size);
>> > > -- 
>> > > 2.47.0
>> > > 
>> > 
>> > I wonder whether 'size < ETH_HLEN' above is needed after your patch.
>> > Feels like 'user_size < ETH_HLEN' supersedes it.
>> 
>> May be fixing it by replacing the existing "size" check with "user_size"
>> check? Seems more intuitive that checking is needed on the "user_"size
>> instead of the "size". The "if (user_size > size)" check looks useless also.
>> Something like this?
>> 
>> -	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);
>> -
>> 
> 
> Agreed, that should do it. IIUC, 'user_size > size' only makes sense
> for the bpf_prog_test_run_xdp case and the caller handles this case
> anyway (size > max_data_sz).

Thank you for your comment.  I'll take your suggestion and test it
with the reproducer.

Thanks,
Shigeru