lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
Open Source and information security mailing list archives
| ||
|
Date: Thu, 01 Dec 2022 10:14:35 +0800 From: Schspa Shi <schspa@...il.com> To: asmadeus@...ewreck.org Cc: ericvh@...il.com, lucho@...kov.net, linux_oss@...debyte.com, davem@...emloft.net, edumazet@...gle.com, kuba@...nel.org, pabeni@...hat.com, v9fs-developer@...ts.sourceforge.net, netdev@...r.kernel.org, linux-kernel@...r.kernel.org, syzbot+8f1060e2aaf8ca55220b@...kaller.appspotmail.com Subject: Re: [PATCH v2] 9p/fd: set req refcount to zero to avoid uninitialized usage asmadeus@...ewreck.org writes: > Schspa Shi wrote on Wed, Nov 30, 2022 at 09:08:31PM +0800: >> When the transport layer of fs cancels the request, it is deleted from the >> client side. But the server can send a response with the freed tag. >> >> When the new request allocated, we add it to idr, and use the id form idr >> as tag, which will have the same tag with high probability. Then initialize >> the refcount after adding it to idr. > > ultimately this bug has nothing to do with tag reuse -- we don't > actually need flush at all to trigger it. > > - thread1 starts new request; idr initialized with tag X > - thread2 receives something for tag X, increments refcount before > refcount init > - thread1 resets refcount to two incorrectly > > This could happen on any new message where the server voluntarily sends > a reply with tag X before the request has been sent; in a cyclic model > as suggested in the other thread it would be easy to guess just last+1 > for an hypothetical attacker. > > This scenario with flush is just how syzbot happened to trigger it, but > I think it's just superfluous to this commit message. > > A few more nitpicks on wording below; happy to adjust things myself as I > apply patches but might as well comment first... > >> 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. >> >> There is the logs from syzbot. > > English: Here is ... > >> 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 seems can fix it. > > English: seems to fix it? > (thanks for checking!) > Sorry for my bad english, this patch will 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] == uninitilzed */ > > typo: uninitialized thanks for check. > >> m->rreq = p9_tag_lookup(m->client, m->rc.tag); > > it's not obvious for someone reading this not familiar with 9p that > lookup will increment refcount > >> >> refcount_set(&req->refcount, 2); >> << do response/error >> >> p9_req_put(m->client, m->rreq); >> /* req->refcount == 1 */ >> >> /* req->refcount == 1 */ >> << got a bad refcount >> > > it's not obvious from this going back to thread 1 with a refcount of 1 > would be a bad refcount, either. > One possible scenario would be: > > /* increments uninitalized refcount */ > req = p9_tag_lookup(tag) > 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) > OK, this is more clear. > > >> To fix it, we can initize the refcount to zero before add to idr. >> >> Reported-by: syzbot+8f1060e2aaf8ca55220b@...kaller.appspotmail.com >> > > There should be no empty line between the tags; tags are part of the > "trailer" and some tools handle it as such (like git interpret-trailers), > which would ignore that Reported-by as it is not part of the last block > of text. > Thanks for reminding the format issue here. >> Signed-off-by: Schspa Shi <schspa@...il.com> >> --- >> net/9p/client.c | 4 ++++ >> 1 file changed, 4 insertions(+) >> >> diff --git a/net/9p/client.c b/net/9p/client.c >> index aaa37b07e30a..a72cb597a8ab 100644 >> --- a/net/9p/client.c >> +++ b/net/9p/client.c >> @@ -297,6 +297,10 @@ 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; >> + /* p9_tag_lookup relies on this refcount to be zero to avoid >> + * getting a freed request. > > A freed request would have 0 by definition, if it isn't zero then this > is a newly allocated uninit request, so this comment is incorrect. > > How about: > /* 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. > */ > Your comment is more clear, I will change to this one. >> + */ >> + refcount_set(&req->refcount, 0); >> init_waitqueue_head(&req->wq); >> INIT_LIST_HEAD(&req->req_list); >> -- BRs Schspa Shi
Powered by blists - more mailing lists