Re: [PATCH net] net/9p: fix uninit-value in p9_client_rpc()

[Dominique Martinet]

Nikita Zhandarovich wrote on Fri, Apr 05, 2024 at 04:35:40AM -0700:
> Syzbot with the help of KMSAN reported the following error:
> 
> BUG: KMSAN: uninit-value in trace_9p_client_res include/trace/events/9p.h:146 [inline]
> BUG: KMSAN: uninit-value in p9_client_rpc+0x1314/0x1340 net/9p/client.c:754
>  trace_9p_client_res include/trace/events/9p.h:146 [inline]
>  p9_client_rpc+0x1314/0x1340 net/9p/client.c:754
>  p9_client_create+0x1551/0x1ff0 net/9p/client.c:1031
>  v9fs_session_init+0x1b9/0x28e0 fs/9p/v9fs.c:410
>  v9fs_mount+0xe2/0x12b0 fs/9p/vfs_super.c:122
>  legacy_get_tree+0x114/0x290 fs/fs_context.c:662
>  vfs_get_tree+0xa7/0x570 fs/super.c:1797
>  do_new_mount+0x71f/0x15e0 fs/namespace.c:3352
>  path_mount+0x742/0x1f20 fs/namespace.c:3679
>  do_mount fs/namespace.c:3692 [inline]
>  __do_sys_mount fs/namespace.c:3898 [inline]
>  __se_sys_mount+0x725/0x810 fs/namespace.c:3875
>  __x64_sys_mount+0xe4/0x150 fs/namespace.c:3875
>  do_syscall_64+0xd5/0x1f0
>  entry_SYSCALL_64_after_hwframe+0x6d/0x75
> 
> Uninit was created at:
>  __alloc_pages+0x9d6/0xe70 mm/page_alloc.c:4598
>  __alloc_pages_node include/linux/gfp.h:238 [inline]
>  alloc_pages_node include/linux/gfp.h:261 [inline]
>  alloc_slab_page mm/slub.c:2175 [inline]
>  allocate_slab mm/slub.c:2338 [inline]
>  new_slab+0x2de/0x1400 mm/slub.c:2391
>  ___slab_alloc+0x1184/0x33d0 mm/slub.c:3525
>  __slab_alloc mm/slub.c:3610 [inline]
>  __slab_alloc_node mm/slub.c:3663 [inline]
>  slab_alloc_node mm/slub.c:3835 [inline]
>  kmem_cache_alloc+0x6d3/0xbe0 mm/slub.c:3852
>  p9_tag_alloc net/9p/client.c:278 [inline]
>  p9_client_prepare_req+0x20a/0x1770 net/9p/client.c:641
>  p9_client_rpc+0x27e/0x1340 net/9p/client.c:688
>  p9_client_create+0x1551/0x1ff0 net/9p/client.c:1031
>  v9fs_session_init+0x1b9/0x28e0 fs/9p/v9fs.c:410
>  v9fs_mount+0xe2/0x12b0 fs/9p/vfs_super.c:122
>  legacy_get_tree+0x114/0x290 fs/fs_context.c:662
>  vfs_get_tree+0xa7/0x570 fs/super.c:1797
>  do_new_mount+0x71f/0x15e0 fs/namespace.c:3352
>  path_mount+0x742/0x1f20 fs/namespace.c:3679
>  do_mount fs/namespace.c:3692 [inline]
>  __do_sys_mount fs/namespace.c:3898 [inline]
>  __se_sys_mount+0x725/0x810 fs/namespace.c:3875
>  __x64_sys_mount+0xe4/0x150 fs/namespace.c:3875
>  do_syscall_64+0xd5/0x1f0
>  entry_SYSCALL_64_after_hwframe+0x6d/0x75
> 
> If p9_check_errors() fails early in p9_client_rpc(), req->rc.tag
> will not be properly initialized. However, trace_9p_client_res()
> ends up trying to print it out anyway before p9_client_rpc()
> finishes.

Good find.
Indeed, tc side is setup properly in p9_tag_alloc() but the rc side can
be left uninit.

Given we do initialize tc side perhaps it's best to initialize rc
similarly in p9_tag_alloc() (eh, there's also some initialization done
in p9pdu_reset that's not used anywhere else... this code could use some
cleanup too), but that's probably overoptimizing it, happy to roll with
that.

I'd appreciate if we could make these initial values something invalid
though -- there is no '0' value for id in the protocol so that one is
fine, but tag=0 is going to be very common so let's initialize it as
P9_NOTAG instead.

I'll move p9pdu_reset code to p9_fcall_init in a later non-fix commit
after you send v2.

> 
> Fix this issue by assigning default values to p9_fcall fields
> such as 'tag' and (just in case KMSAN unearths something new) 'id'
> during the tag allocation stage.
> 
> Reported-and-tested-by: syzbot+ff14db38f56329ef68df@xxxxxxxxxxxxxxxxxxxxxxxxx
> Fixes: 348b59012e5c ("net/9p: Convert net/9p protocol dumps to tracepoints")
> Signed-off-by: Nikita Zhandarovich <n.zhandarovich@xxxxxxxxxx>
> ---
> P.S. Not entirely sure that 'Fixes' tag is fully correct here.

Right there's been quite some rework there, the probe has been added at
this point and I believe the bug has always been present from a quick
look at the code so it's proably correct, but it might not be easy to
backport.
Let's leave it as is.

Thanks,
-- 
Dominique Martinet | Asmadeus