Messages in this thread | | | From | Christian Schoenebeck <> | Subject | Re: [PATCH v3] 9p/fd: set req refcount to zero to avoid uninitialized usage | Date | Fri, 02 Dec 2022 12:48:39 +0100 |
| |
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@syzkaller.appspotmail.com > Signed-off-by: Schspa Shi <schspa@gmail.com>
Reviewed-by: Christian Schoenebeck <linux_oss@crudebyte.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); > >
| |