KASAN: invalid-free in p9stat_free
Subsystems (3)
syzbot
syzbotHello,
syzbot found the following crash on:
HEAD commit: e27bc174c9c6 Add linux-next specific files for 20180824 git tree: linux-next console output: https://syzkaller.appspot.com/x/log.txt?x=15dc19a6400000 kernel config: https://syzkaller.appspot.com/x/.config?x=28446088176757ea dashboard link: https://syzkaller.appspot.com/bug?extid=d4252148d198410b864f compiler: gcc (GCC) 8.0.1 20180413 (experimental) syz repro: https://syzkaller.appspot.com/x/repro.syz?x=15f8efba400000 C reproducer: https://syzkaller.appspot.com/x/repro.c?x=1178256a400000
IMPORTANT: if you fix the bug, please add the following tag to the commit:
random: sshd: uninitialized urandom read (32 bytes read)
random: sshd: uninitialized urandom read (32 bytes read)
random: sshd: uninitialized urandom read (32 bytes read)
random: sshd: uninitialized urandom read (32 bytes read)
==================================================================
BUG: KASAN: double-free or invalid-free in p9stat_free+0x35/0x100 net/9p/protocol.c:48 CPU: 0 PID: 4499 Comm: syz-executor922 Not tainted 4.18.0-next-20180824+ #47 Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011 Call Trace: __dump_stack lib/dump_stack.c:77 [inline] dump_stack+0x1c9/0x2b4 lib/dump_stack.c:113 print_address_description+0x6c/0x20b mm/kasan/report.c:256 kasan_report_invalid_free+0x64/0xa0 mm/kasan/report.c:336 __kasan_slab_free+0x150/0x170 mm/kasan/kasan.c:501 kasan_slab_free+0xe/0x10 mm/kasan/kasan.c:528 __cache_free mm/slab.c:3498 [inline] kfree+0xd9/0x210 mm/slab.c:3813 p9stat_free+0x35/0x100 net/9p/protocol.c:48 v9fs_dir_readdir+0x68e/0xbc0 fs/9p/vfs_dir.c:153 iterate_dir+0x48b/0x5d0 fs/readdir.c:51 __do_sys_getdents fs/readdir.c:231 [inline] __se_sys_getdents fs/readdir.c:212 [inline] __x64_sys_getdents+0x29f/0x510 fs/readdir.c:212 do_syscall_64+0x1b9/0x820 arch/x86/entry/common.c:290 entry_SYSCALL_64_after_hwframe+0x49/0xbe RIP: 0033:0x4406a9 Code: 18 89 d0 c3 66 2e 0f 1f 84 00 00 00 00 00 0f 1f 00 48 89 f8 48 89 f7 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff ff 0f 83 fb 13 fc ff c3 66 2e 0f 1f 84 00 00 00 00 RSP: 002b:00007fffc1b13808 EFLAGS: 00000217 ORIG_RAX: 000000000000004e RAX: ffffffffffffffda RBX: 0030656c69662f2e RCX: 00000000004406a9 RDX: 0000000000000008 RSI: 0000000020000180 RDI: 0000000000000005 RBP: 64663d736e617274 R08: 0000000000401f30 R09: 0000000000401f30 R10: 0000000000401f30 R11: 0000000000000217 R12: 0000000000401f30 R13: 0000000000401fc0 R14: 0000000000000000 R15: 0000000000000000 Allocated by task 4499: save_stack+0x43/0xd0 mm/kasan/kasan.c:448 set_track mm/kasan/kasan.c:460 [inline] kasan_kmalloc+0xc4/0xe0 mm/kasan/kasan.c:553 __do_kmalloc mm/slab.c:3718 [inline] __kmalloc+0x14e/0x720 mm/slab.c:3727 kmalloc include/linux/slab.h:518 [inline] p9pdu_vreadf net/9p/protocol.c:157 [inline] p9pdu_readf+0x526/0x2170 net/9p/protocol.c:536 p9pdu_vreadf net/9p/protocol.c:208 [inline] p9pdu_readf+0xd5c/0x2170 net/9p/protocol.c:536 p9stat_read+0x194/0x5d0 net/9p/protocol.c:565 v9fs_dir_readdir+0x63d/0xbc0 fs/9p/vfs_dir.c:149 iterate_dir+0x48b/0x5d0 fs/readdir.c:51 __do_sys_getdents fs/readdir.c:231 [inline] __se_sys_getdents fs/readdir.c:212 [inline] __x64_sys_getdents+0x29f/0x510 fs/readdir.c:212 do_syscall_64+0x1b9/0x820 arch/x86/entry/common.c:290 entry_SYSCALL_64_after_hwframe+0x49/0xbe Freed by task 4499: save_stack+0x43/0xd0 mm/kasan/kasan.c:448 set_track mm/kasan/kasan.c:460 [inline] __kasan_slab_free+0x11a/0x170 mm/kasan/kasan.c:521 kasan_slab_free+0xe/0x10 mm/kasan/kasan.c:528 __cache_free mm/slab.c:3498 [inline] kfree+0xd9/0x210 mm/slab.c:3813 p9stat_free+0x35/0x100 net/9p/protocol.c:48 p9pdu_vreadf net/9p/protocol.c:220 [inline] p9pdu_readf+0xd90/0x2170 net/9p/protocol.c:536 p9stat_read+0x194/0x5d0 net/9p/protocol.c:565 v9fs_dir_readdir+0x63d/0xbc0 fs/9p/vfs_dir.c:149 iterate_dir+0x48b/0x5d0 fs/readdir.c:51 __do_sys_getdents fs/readdir.c:231 [inline] __se_sys_getdents fs/readdir.c:212 [inline] __x64_sys_getdents+0x29f/0x510 fs/readdir.c:212 do_syscall_64+0x1b9/0x820 arch/x86/entry/common.c:290 entry_SYSCALL_64_after_hwframe+0x49/0xbe The buggy address belongs to the object at ffff8801b3006700 which belongs to the cache kmalloc-32 of size 32 The buggy address is located 0 bytes inside of 32-byte region [ffff8801b3006700, ffff8801b3006720) The buggy address belongs to the page: page:ffffea0006cc0180 count:1 mapcount:0 mapping:ffff8801dac001c0 index:0xffff8801b3006fc1 flags: 0x2fffc0000000100(slab) raw: 02fffc0000000100 ffff8801dac01238 ffffea0006cc6548 ffff8801dac001c0 raw: ffff8801b3006fc1 ffff8801b3006000 0000000100000037 0000000000000000 page dumped because: kasan: bad access detected Memory state around the buggy address: ffff8801b3006600: fb fb fb fb fc fc fc fc fb fb fb fb fc fc fc fc ffff8801b3006680: fb fb fb fb fc fc fc fc fb fb fb fb fc fc fc fc
Quoted 1 line
ffff8801b3006700: fb fb fb fb fc fc fc fc fb fb fb fb fc fc fc fc
^
ffff8801b3006780: fb fb fb fb fc fc fc fc fb fb fb fb fc fc fc fc
ffff8801b3006800: fb fb fb fb fc fc fc fc 05 fc fc fc fc fc fc fc
==================================================================
---
This bug is generated by a bot. It may contain errors. See https://goo.gl/tpsmEJ for more information about syzbot. syzbot engineers can be reached at [email protected].
syzbot will keep track of this bug report. See:
https://goo.gl/tpsmEJ#bug-status-tracking for how to communicate with syzbot. syzbot can test patches for this bug, for details see:
https://goo.gl/tpsmEJ#testing-patches
Dominique Martinet
maintainersyzbot wrote on Sun, Aug 26, 2018:
Quoted 12 lines
HEAD commit: e27bc174c9c6 Add linux-next specific files for 20180824 git tree: linux-next console output: https://syzkaller.appspot.com/x/log.txt?x=15dc19a6400000 kernel config: https://syzkaller.appspot.com/x/.config?x=28446088176757ea dashboard link: https://syzkaller.appspot.com/bug?extid=d4252148d198410b864f compiler: gcc (GCC) 8.0.1 20180413 (experimental) syz repro: https://syzkaller.appspot.com/x/repro.syz?x=15f8efba400000 C reproducer: https://syzkaller.appspot.com/x/repro.c?x=1178256a400000IMPORTANT: if you fix the bug, please add the following tag to the commit:
Reported-by [email protected]
Quoted 7 lines
random: sshd: uninitialized urandom read (32 bytes read)
random: sshd: uninitialized urandom read (32 bytes read)
random: sshd: uninitialized urandom read (32 bytes read)
random: sshd: uninitialized urandom read (32 bytes read)
==================================================================BUG: KASAN: double-free or invalid-free in p9stat_free+0x35/0x100
net/9p/protocol.c:48
That looks straight-forward enough, p9pdu_vreadf does p9stat_free on error then v9fs_dir_readdir does the same ; there is nothing else that could return an error without going through the first free so we could just remove the later one...
There are a couple other users of the 'S' pdu read (that reads the stat struct and frees it on error), so it's probably best to keep the current behaviour as far as this is concerned, what we could do though is make the free function idempotent (write NULLs in the freed fields), but I do not see this being done often, do you know what the policy is about this kind of pattern nowadays?
The struct is cleanly zeroed before being read so there is no risk of double-frees between iterations so zeroing pointers is not strictly required, but it does make things safer in general.
Signature · 1 line
Dominique Martinet
Quoted 26 lines · Dominique Martinet
syzbot wrote on Sun, Aug 26, 2018:
Quoted 12 lines
HEAD commit: e27bc174c9c6 Add linux-next specific files for 20180824 git tree: linux-next console output: https://syzkaller.appspot.com/x/log.txt?x=15dc19a6400000 kernel config: https://syzkaller.appspot.com/x/.config?x=28446088176757ea dashboard link: https://syzkaller.appspot.com/bug?extid=d4252148d198410b864f compiler: gcc (GCC) 8.0.1 20180413 (experimental) syz repro: https://syzkaller.appspot.com/x/repro.syz?x=15f8efba400000 C reproducer: https://syzkaller.appspot.com/x/repro.c?x=1178256a400000IMPORTANT: if you fix the bug, please add the following tag to the commit:
Reported-by [email protected]Quoted 7 lines
random: sshd: uninitialized urandom read (32 bytes read)
random: sshd: uninitialized urandom read (32 bytes read)
random: sshd: uninitialized urandom read (32 bytes read)
random: sshd: uninitialized urandom read (32 bytes read)
==================================================================BUG: KASAN: double-free or invalid-free in p9stat_free+0x35/0x100
net/9p/protocol.c:48That looks straight-forward enough,
p9pdu_vreadfdoesp9stat_freeon error thenv9fs_dir_readdirdoes the same ; there is nothing else that could return an error without going through the first free so we could just remove the later one...
Quoted 6 lines
There are a couple other users of the 'S' pdu read (that reads the stat struct and frees it on error), so it's probably best to keep the current behaviour as far as this is concerned, what we could do though is make the free function idempotent (write NULLs in the freed fields), but I do not see this being done often, do you know what the policy is about this kind of pattern nowadays?
Hi Dominique,
kfree and then null pointer is pretty common, try to run:
find -name "*.c" -exec grep -A 1 "kfree(" {} \; | grep -B 1 " = NULL;"
Leaving dangling pointers behind is not the best idea.
And from what I remember a bunch of similar double frees were fixed by nulling the pointer after the first kfree.
Quoted 3 lines
The struct is cleanly zeroed before being read so there is no risk of double-frees between iterations so zeroing pointers is not strictly required, but it does make things safer in general.
Dominique Martinet
maintainerDmitry Vyukov wrote on Mon, Aug 27, 2018:
Quoted 3 lines
kfree and then null pointer is pretty common, try to run:
find -name "*.c" -exec grep -A 1 "kfree(" {} \; | grep -B 1 " = NULL;"
Hmm, right, it looks like somewhere between 5 and 10% of the kfree() calls are followed by NULL assignment, that's "common enough" - not generalized but not rare either.
Quoted 3 lines
Leaving dangling pointers behind is not the best idea.
And from what I remember a bunch of similar double frees were fixed by nulling the pointer after the first kfree.
In this case it really is an error to call p9stat_free again, so let's just do both.
Will send the patches shortly.
Thanks,
Signature · 1 line
Dominique
Dominique Martinet
maintainerPATCH 1/2 v9fs_dir_readdir: fix double-free on p9stat_read error
In reply to syzbotFrom: Dominique Martinet <[email protected]>
p9stat_read will call p9stat_free on error, we should only free the struct content on success.
There also is no need to "p9stat_init" st as the read function will zero the whole struct for us anyway, so clean up the code a bit while we are here.
--- fs/9p/vfs_dir.c | 11 ----------- 1 file changed, 11 deletions(-)
fs/9p/vfs_dir.c +0 -11
@@ -76,15 +76,6 @@ static inline int dt_type(struct p9_wstat *mistat)return rettype; } -static void p9stat_init (struct p9_wstat *stbuf) -{ - stbuf->name =NULL ; - stbuf->uid =NULL ; - stbuf->gid =NULL ; - stbuf->muid =NULL ; - stbuf->extension =NULL ; -} -/** * v9fs_alloc_rdir_buf - Allocate buffer used for read and readdir * @filp: opened file structure
@@ -145,12 +136,10 @@ static int v9fs_dir_readdir(struct file *file, struct dir_context *ctx) rdir->tail = n; }while (rdir->head < rdir->tail) { -p9stat_init (&st); err =p9stat_read (fid->clnt, rdir->buf + rdir->head, rdir->tail - rdir->head, &st);if (err) {p9_debug (P9_DEBUG_VFS ,"returned %d\n" , err); -p9stat_free (&st);return -EIO ; } reclen = st.size+2 ;
Signature · 1 line
2.17.1
Dominique Martinet
maintainerPATCH 2/2 9p: clear dangling pointers in p9stat_free
From: Dominique Martinet <[email protected]>
p9stat_free is more of a cleanup function than a 'free' function as it only frees the content of the struct; there are chances of use-after-free if it is improperly used (e.g. p9stat_free called twice as it used to be possible to)
Clearing dangling pointers makes the function idempotent and safer to use.
--- net/9p/protocol.c | 5 +++++ 1 file changed, 5 insertions(+)
net/9p/protocol.c +5 -0
@@ -46,10 +46,15 @@ p9pdu_writef(struct p9_fcall *pdu, int proto_version, const char *fmt, ...);void p9stat_free (struct p9_wstat *stbuf) {kfree (stbuf->name); + stbuf->name =NULL ;kfree (stbuf->uid); + stbuf->uid =NULL ;kfree (stbuf->gid); + stbuf->gid =NULL ;kfree (stbuf->muid); + stbuf->muid =NULL ;kfree (stbuf->extension); + stbuf->extension =NULL ; }EXPORT_SYMBOL (p9stat_free);
Signature · 1 line
2.17.1