Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
References: <20221130130830.97199-1-schspa@gmail.com>
 <Y4da9/BHrEqgwq4q@codewreck.org>
User-agent: mu4e 1.8.10; emacs 28.2
From:   Schspa Shi <schspa@gmail.com>
To:     asmadeus@codewreck.org
Cc:     ericvh@gmail.com, lucho@ionkov.net, linux_oss@crudebyte.com,
        davem@davemloft.net, edumazet@google.com, kuba@kernel.org,
        pabeni@redhat.com, v9fs-developer@lists.sourceforge.net,
        netdev@vger.kernel.org, linux-kernel@vger.kernel.org,
        syzbot+8f1060e2aaf8ca55220b@syzkaller.appspotmail.com
Subject: Re: [PATCH v2] 9p/fd: set req refcount to zero to avoid
 uninitialized usage
Date:   Thu, 01 Dec 2022 10:14:35 +0800
In-reply-to: <Y4da9/BHrEqgwq4q@codewreck.org>
Message-ID: <m2r0xj6f2x.fsf@gmail.com>
MIME-Version: 1.0
Content-Type: text/plain
Precedence: bulk


asmadeus@codewreck.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@syzkaller.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@gmail.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