| Message ID | 20240405113540.20456-1-n.zhandarovich@fintech.ru (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [net] net/9p: fix uninit-value in p9_client_rpc() | expand |
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@syzkaller.appspotmail.com > Fixes: 348b59012e5c ("net/9p: Convert net/9p protocol dumps to tracepoints") > Signed-off-by: Nikita Zhandarovich <n.zhandarovich@fintech.ru> > --- > 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,
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; }
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: syzbot+ff14db38f56329ef68df@syzkaller.appspotmail.com Fixes: 348b59012e5c ("net/9p: Convert net/9p protocol dumps to tracepoints") Signed-off-by: Nikita Zhandarovich <n.zhandarovich@fintech.ru> --- P.S. Not entirely sure that 'Fixes' tag is fully correct here. net/9p/client.c | 2 ++ 1 file changed, 2 insertions(+)