* [PATCH v2] 9p/fd: set req refcount to zero to avoid uninitialized usage
@ 2022-11-30 13:08 Schspa Shi
2022-11-30 13:30 ` asmadeus
0 siblings, 1 reply; 3+ messages in thread
From: Schspa Shi @ 2022-11-30 13:08 UTC (permalink / raw)
To: ericvh, lucho, asmadeus, linux_oss, davem, edumazet, kuba, pabeni
Cc: v9fs-developer, netdev, linux-kernel, Schspa Shi,
syzbot+8f1060e2aaf8ca55220b
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.
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.
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.
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 */
m->rreq = p9_tag_lookup(m->client, m->rc.tag);
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 >>
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>
---
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.
+ */
+ refcount_set(&req->refcount, 0);
init_waitqueue_head(&req->wq);
INIT_LIST_HEAD(&req->req_list);
--
2.37.3
^ permalink raw reply related [flat|nested] 3+ messages in thread* Re: [PATCH v2] 9p/fd: set req refcount to zero to avoid uninitialized usage
2022-11-30 13:08 [PATCH v2] 9p/fd: set req refcount to zero to avoid uninitialized usage Schspa Shi
@ 2022-11-30 13:30 ` asmadeus
2022-12-01 2:14 ` Schspa Shi
0 siblings, 1 reply; 3+ messages in thread
From: asmadeus @ 2022-11-30 13:30 UTC (permalink / raw)
To: Schspa Shi
Cc: ericvh, lucho, linux_oss, davem, edumazet, kuba, pabeni,
v9fs-developer, netdev, linux-kernel, syzbot+8f1060e2aaf8ca55220b
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!)
>
> 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
> 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)
> 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.
> 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.
*/
> + */
> + refcount_set(&req->refcount, 0);
> init_waitqueue_head(&req->wq);
> INIT_LIST_HEAD(&req->req_list);
>
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [PATCH v2] 9p/fd: set req refcount to zero to avoid uninitialized usage
2022-11-30 13:30 ` asmadeus
@ 2022-12-01 2:14 ` Schspa Shi
0 siblings, 0 replies; 3+ messages in thread
From: Schspa Shi @ 2022-12-01 2:14 UTC (permalink / raw)
To: asmadeus
Cc: ericvh, lucho, linux_oss, davem, edumazet, kuba, pabeni,
v9fs-developer, netdev, linux-kernel, syzbot+8f1060e2aaf8ca55220b
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
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2022-12-01 2:19 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2022-11-30 13:08 [PATCH v2] 9p/fd: set req refcount to zero to avoid uninitialized usage Schspa Shi
2022-11-30 13:30 ` asmadeus
2022-12-01 2:14 ` Schspa Shi
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).