KASAN: invalid-free in p9stat_free

[syzbot]

Hello,

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:
Reported-by: syzbot+d42521...@syzkaller.appspotmail.com

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
> 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 syzk...@googlegroups.com.

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]

syzbot wrote on Sun, Aug 26, 2018:
> 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:
> Reported-by: syzbot+d42521...@syzkaller.appspotmail.com
>
> 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.


--
Dominique Martinet

[Dmitry Vyukov]

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.

[Dominique Martinet]

Dmitry Vyukov wrote on Mon, Aug 27, 2018:
> 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.

> 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,
--
Dominique

[Dominique Martinet]

From: Dominique Martinet <dominique...@cea.fr>

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.

Signed-off-by: Dominique Martinet <dominique...@cea.fr>
Reported-by: syzbot+d42521...@syzkaller.appspotmail.com
Cc: Eric Van Hensbergen <eri...@gmail.com>
Cc: Latchesar Ionkov <lu...@ionkov.net>
---
fs/9p/vfs_dir.c | 11 -----------
1 file changed, 11 deletions(-)

diff --git a/fs/9p/vfs_dir.c b/fs/9p/vfs_dir.c
index b0405d6aac85..48db9a9f13f9 100644
--- a/fs/9p/vfs_dir.c
+++ b/fs/9p/vfs_dir.c
@@ -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;
--
2.17.1

[Dominique Martinet]

From: Dominique Martinet <dominique...@cea.fr>

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.

Signed-off-by: Dominique Martinet <dominique...@cea.fr>
Reported-by: syzbot+d42521...@syzkaller.appspotmail.com
Cc: Eric Van Hensbergen <eri...@gmail.com>
Cc: Latchesar Ionkov <lu...@ionkov.net>
---

net/9p/protocol.c | 5 +++++
1 file changed, 5 insertions(+)

diff --git a/net/9p/protocol.c b/net/9p/protocol.c
index 4a1e1dd30b52..ee32bbf12675 100644
--- a/net/9p/protocol.c
+++ b/net/9p/protocol.c
@@ -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);

--
2.17.1