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.
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: [email protected]
Fixes: 348b59012e5c ("net/9p: Convert net/9p protocol dumps to tracepoints")
Signed-off-by: Nikita Zhandarovich <[email protected]>
---
P.S. Not entirely sure that 'Fixes' tag is fully correct here.
net/9p/client.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/net/9p/client.c b/net/9p/client.c
index e265a0ca6bdd..a9d613af7455 100644
--- a/net/9p/client.c
+++ b/net/9p/client.c
@@ -235,6 +235,8 @@ static int p9_fcall_init(struct p9_client *c, struct p9_fcall *fc,
if (!fc->sdata)
return -ENOMEM;
fc->capacity = alloc_msize;
+ fc->id = 0;
+ fc->tag = 0;
return 0;
}
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: [email protected]
> Fixes: 348b59012e5c ("net/9p: Convert net/9p protocol dumps to tracepoints")
> Signed-off-by: Nikita Zhandarovich <[email protected]>
> ---
> 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