netdev - Re: [PATCH bpf] bpf, test_run: Fix use-after-free issue in eth_skb_pkt

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-Id: <20241203.225956.138899513616764420.syoshida@redhat.com>
Date: Tue, 03 Dec 2024 22:59:56 +0900 (JST)
From: Shigeru Yoshida <syoshida@...hat.com>
To: stfomichev@...il.com, martin.lau@...ux.dev
Cc: ast@...nel.org, daniel@...earbox.net, andrii@...nel.org,
 eddyz87@...il.com, song@...nel.org, yonghong.song@...ux.dev,
 john.fastabend@...il.com, kpsingh@...nel.org, sdf@...ichev.me,
 haoluo@...gle.com, jolsa@...nel.org, davem@...emloft.net,
 edumazet@...gle.com, kuba@...nel.org, pabeni@...hat.com, horms@...nel.org,
 hawk@...nel.org, lorenzo@...nel.org, toke@...hat.com, bpf@...r.kernel.org,
 netdev@...r.kernel.org, syzkaller@...glegroups.com
Subject: Re: [PATCH bpf] bpf, test_run: Fix use-after-free issue in
 eth_skb_pkt_type()

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@...glegroups.com>
>> > > Signed-off-by: Shigeru Yoshida <syoshida@...hat.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