[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <4759293.MmlG3nAkEO@silver>
Date: Fri, 02 Dec 2022 12:48:39 +0100
From: Christian Schoenebeck <linux_oss@...debyte.com>
To: ericvh@...il.com, lucho@...kov.net, asmadeus@...ewreck.org,
davem@...emloft.net, edumazet@...gle.com, kuba@...nel.org,
pabeni@...hat.com, Schspa Shi <schspa@...il.com>
Cc: v9fs-developer@...ts.sourceforge.net, netdev@...r.kernel.org,
linux-kernel@...r.kernel.org, Schspa Shi <schspa@...il.com>,
syzbot+8f1060e2aaf8ca55220b@...kaller.appspotmail.com
Subject: Re: [PATCH v3] 9p/fd: set req refcount to zero to avoid uninitialized usage
On Thursday, December 1, 2022 4:33:10 AM CET Schspa Shi wrote:
> When the new request allocated, the refcount will be zero if it is resued
> one. But if the request is newly allocated from slab, it is not fully
> initialized before add it to idr.
>
> If the p9_read_work got a response before the refcount initiated. It will
> use a uninitialized req, which will result in a bad request data struct.
>
> Here is the logs from syzbot.
>
> Corrupted memory at 0xffff88807eade00b [ 0xff 0x07 0x00 0x00 0x00 0x00
> 0x00 0x00 . . . . . . . . ] (in kfence-#110):
> p9_fcall_fini net/9p/client.c:248 [inline]
> p9_req_put net/9p/client.c:396 [inline]
> p9_req_put+0x208/0x250 net/9p/client.c:390
> p9_client_walk+0x247/0x540 net/9p/client.c:1165
> clone_fid fs/9p/fid.h:21 [inline]
> v9fs_fid_xattr_set+0xe4/0x2b0 fs/9p/xattr.c:118
> v9fs_xattr_set fs/9p/xattr.c:100 [inline]
> v9fs_xattr_handler_set+0x6f/0x120 fs/9p/xattr.c:159
> __vfs_setxattr+0x119/0x180 fs/xattr.c:182
> __vfs_setxattr_noperm+0x129/0x5f0 fs/xattr.c:216
> __vfs_setxattr_locked+0x1d3/0x260 fs/xattr.c:277
> vfs_setxattr+0x143/0x340 fs/xattr.c:309
> setxattr+0x146/0x160 fs/xattr.c:617
> path_setxattr+0x197/0x1c0 fs/xattr.c:636
> __do_sys_setxattr fs/xattr.c:652 [inline]
> __se_sys_setxattr fs/xattr.c:648 [inline]
> __ia32_sys_setxattr+0xc0/0x160 fs/xattr.c:648
> do_syscall_32_irqs_on arch/x86/entry/common.c:112 [inline]
> __do_fast_syscall_32+0x65/0xf0 arch/x86/entry/common.c:178
> do_fast_syscall_32+0x33/0x70 arch/x86/entry/common.c:203
> entry_SYSENTER_compat_after_hwframe+0x70/0x82
>
> Below is a similar scenario, the scenario in the syzbot log looks more
> complicated than this one, but this patch can fix it.
>
> T21124 p9_read_work
> ======================== second trans =================================
> p9_client_walk
> p9_client_rpc
> p9_client_prepare_req
> p9_tag_alloc
> req = kmem_cache_alloc(p9_req_cache, GFP_NOFS);
> tag = idr_alloc
> << preempted >>
> req->tc.tag = tag;
> /* req->[refcount/tag] == uninitialized */
> m->rreq = p9_tag_lookup(m->client, m->rc.tag);
> /* increments uninitalized refcount */
>
> refcount_set(&req->refcount, 2);
> /* cb drops one ref */
> p9_client_cb(req)
> /* reader thread drops its ref:
> request is incorrectly freed */
> p9_req_put(req)
> /* use after free and ref underflow */
> p9_req_put(req)
>
> To fix it, we can initize the refcount to zero before add to idr.
>
> Reported-by: syzbot+8f1060e2aaf8ca55220b@...kaller.appspotmail.com
> Signed-off-by: Schspa Shi <schspa@...il.com>
Reviewed-by: Christian Schoenebeck <linux_oss@...debyte.com>
>
> --
>
> Changelog:
> v1 -> v2:
> - Set refcount to fix the problem.
> v2 -> v3:
> - Comment messages improve as asmadeus suggested.
> ---
> net/9p/client.c | 5 +++++
> 1 file changed, 5 insertions(+)
>
> diff --git a/net/9p/client.c b/net/9p/client.c
> index aaa37b07e30a..ec74cd29d3bc 100644
> --- a/net/9p/client.c
> +++ b/net/9p/client.c
> @@ -297,6 +297,11 @@ p9_tag_alloc(struct p9_client *c, int8_t type, uint t_size, uint r_size,
> p9pdu_reset(&req->rc);
> req->t_err = 0;
> req->status = REQ_STATUS_ALLOC;
> + /* refcount needs to be set to 0 before inserting into the idr
> + * so p9_tag_lookup does not accept a request that is not fully
> + * initialized. refcount_set to 2 below will mark request live.
> + */
> + refcount_set(&req->refcount, 0);
I would s/live/ready for being used/, but comment should be clear enough
anyway.
> init_waitqueue_head(&req->wq);
> INIT_LIST_HEAD(&req->req_list);
>
>
Powered by blists - more mailing lists