Messages in this thread | | | From | Schspa Shi <> | Subject | Re: [PATCH] 9p: fix crash when transaction killed | Date | Wed, 30 Nov 2022 11:26:02 +0800 |
| |
Schspa Shi <schspa@gmail.com> writes:
> asmadeus@codewreck.org writes: > >> Schspa Shi wrote on Wed, Nov 30, 2022 at 12:22:51AM +0800: >>> The transport layer of fs does not fully support the cancel request. >>> When the request is in the REQ_STATUS_SENT state, p9_fd_cancelled >>> will forcibly delete the request, and at this time p9_[read/write]_work >>> may continue to use the request. Therefore, it causes UAF . >>> >>> There 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 the root cause seems to be the same. >>> >>> T21124 p9_write_work p9 read_work >>> ======================== first trans ================================= >>> p9_client_walk >>> p9_client_rpc >>> p9_client_prepare_req >>> /* req->refcount == 2 */ >>> c->trans_mod->request(c, req); >>> p9_fd_request >>> req move to unsent_req_list >>> req->status = REQ_STATUS_SENT; >>> req move to req_list >>> << send to server >> >>> wait_event_killable >>> << get kill signal >> >>> if (c->trans_mod->cancel(c, req)) >>> p9_client_flush(c, req); >>> /* send flush request */ >>> req = p9_client_rpc(c, P9_TFLUSH, "w", oldtag); >>> if (c->trans_mod->cancelled) >>> c->trans_mod->cancelled(c, oldreq); >>> /* old req was deleted from req_list */ >>> /* req->refcount == 1 */ >>> p9_req_put >>> /* req->refcount == 0 */ >>> << preempted >> >>> << get response, UAF here >> >>> m->rreq = p9_tag_lookup(m->client, m->rc.tag); >>> /* req->refcount == 1 */ >>> << do response >> >>> p9_client_cb(m->client, m->rreq, REQ_STATUS_RCVD); >>> /* req->refcount == 0 */ >>> p9_fcall_fini >>> /* request have been freed */ >>> p9_fcall_fini >>> /* double free */ >>> p9_req_put(m->client, m->rreq); >>> /* req->refcount == 1 */ >>> >>> To fix it, we can wait the request with status REQ_STATUS_SENT returned. >> >> Christian replied on this (we cannot wait) but I agree with him -- the > > Yes, this is where I worry about too, this wait maybe cause a deadlock. >
@Christian: It seems we don't need this wait, The problem maybe cause by lack of lock in p9_tag_lookup.
>> scenario you describe is proteced by p9_tag_lookup checking for refcount >> with refcount_inc_not_zero (p9_req_try_get). > > Thanks for pointing out the zero value check here, the scene in the > commit message does not hold. > >> >> The normal scenarii for flush are as follow: >> - cancel before request is sent: no flush, just free >> - flush is ignored and reply comes first: we get reply from original >> request then reply from flush >> - flush is handled and reply never comes: we only get reply from flush >> >> Protocol-wise, we can safely reuse the tag after the flush reply got >> received; and as far as I can follow the code we only ever free the tag >> (last p9_call_fini) after flush has returned so the entry should be >> protected. >> >> If we receive a response on the given tag between cancelled and the main >> thread going out the request has been marked as FLSHD and should be >> ignored. . . here is one p9_req_put in p9_read_work() in this case but >> it corresponds to the ref obtained by p9_tag_lookup() so it should be >> valid. >> >> >> I'm happy to believe we have a race somewhere (even if no sane server >> would produce it), but right now I don't see it looking at the code.. :/ > > And I think there is a race too. because the syzbot report about 9p fs > memory corruption multi times. > > As for the problem, the p9_tag_lookup only takes the rcu_read_lock when > accessing the IDR, why it doesn't take the p9_client->lock? Maybe the > root cause is that a lock is missing here.
Add Christian Schoenebeck for bad mail address typo.
-- BRs Schspa Shi
| |