lkml.org 
[lkml]   [2022]   [Dec]   [4]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
SubjectRe: [PATCH v3] 9p/fd: set req refcount to zero to avoid uninitialized usage
Date

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

\
 
 \ /
  Last update: 2022-12-04 15:40    [W:0.034 / U:0.076 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site