[PATCH v3] 9p/fd: set req refcount to zero to avoid uninitialized usage

[PATCH v3] 9p/fd: set req refcount to zero to avoid uninitialized usage

[Schspa Shi]

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>

--

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);
 	init_waitqueue_head(&req->wq);
 	INIT_LIST_HEAD(&req->req_list);
 
-- 
2.37.3

Re: [PATCH v3] 9p/fd: set req refcount to zero to avoid uninitialized usage

[Christian Schoenebeck]

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);
>  
> 

Re: [PATCH v3] 9p/fd: set req refcount to zero to avoid uninitialized usage

[asmadeus]

Schspa Shi wrote on Thu, Dec 01, 2022 at 11:33:10AM +0800:
> 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.

(fixed initialize typo here)

> Reported-by: syzbot+8f1060e2aaf8ca55220b@syzkaller.appspotmail.com
> Signed-off-by: Schspa Shi <schspa@gmail.com>
> 
> --
> 
> Changelog:
> v1 -> v2:
>         - Set refcount to fix the problem.
> v2 -> v3:
>         - Comment messages improve as asmadeus suggested.

Just a note: when applying a patch with git am, this goes into the
commit message -- please include the changelog below the git's three
dashes instead (anything between the three dashes and the 'diff --git'
below:
> ---
>  net/9p/client.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/net/9p/client.c b/net/9p/client.c


Christian Schoenebeck wrote on Fri, Dec 02, 2022 at 12:48:39PM +0100:
> > +	/* 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.

I blame golfing to fit into three lines, sorry!
Since it was my suggestion, I've taken the liberty to change 'live' to
'ready' as an half step; I think it's clearer than live and probably
understandable enough.

I've pushed this to my next branch and will submit to Linus for the
merge window in a couple of weeks, no point in rushing this to stable
unless it gets snatched through the net tree first...

-- 
Dominique

Re: [PATCH v3] 9p/fd: set req refcount to zero to avoid uninitialized usage

[Schspa Shi]

asmadeus@codewreck.org writes:

> Schspa Shi wrote on Thu, Dec 01, 2022 at 11:33:10AM +0800:
>> 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.
>
> (fixed initialize typo here)
>
>> Reported-by: syzbot+8f1060e2aaf8ca55220b@syzkaller.appspotmail.com
>> Signed-off-by: Schspa Shi <schspa@gmail.com>
>> 
>> --
>> 
>> Changelog:
>> v1 -> v2:
>>         - Set refcount to fix the problem.
>> v2 -> v3:
>>         - Comment messages improve as asmadeus suggested.
>
> Just a note: when applying a patch with git am, this goes into the
> commit message -- please include the changelog below the git's three
> dashes instead (anything between the three dashes and the 'diff --git'
> below:

Thanks for the reminder, I will pay attention to this next time.

>> ---
>>  net/9p/client.c | 5 +++++
>>  1 file changed, 5 insertions(+)
>> 
>> diff --git a/net/9p/client.c b/net/9p/client.c
>
>
> Christian Schoenebeck wrote on Fri, Dec 02, 2022 at 12:48:39PM +0100:
>> > +	/* 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.
>
> I blame golfing to fit into three lines, sorry!
> Since it was my suggestion, I've taken the liberty to change 'live' to
> 'ready' as an half step; I think it's clearer than live and probably
> understandable enough.
>
> I've pushed this to my next branch and will submit to Linus for the
> merge window in a couple of weeks, no point in rushing this to stable
> unless it gets snatched through the net tree first...

Thanks.

-- 
BRs
Schspa Shi