KASAN: invalid-free in p9stat_free

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+d4252148d198410b864f@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 syzkaller@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

Re: KASAN: invalid-free in p9stat_free

[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+d4252148d198410b864f@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

Re: KASAN: invalid-free in p9stat_free

[Dmitry Vyukov]

On Sun, Aug 26, 2018 at 10:24 PM, Dominique Martinet
<asmadeus@codewreck.org> wrote:
> 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+d4252148d198410b864f@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?


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.


> 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.

Re: KASAN: invalid-free in p9stat_free

[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

[PATCH 1/2] v9fs_dir_readdir: fix double-free on p9stat_read error

[Dominique Martinet]

From: Dominique Martinet <dominique.martinet@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.martinet@cea.fr>
Reported-by: syzbot+d4252148d198410b864f@syzkaller.appspotmail.com
Cc: Eric Van Hensbergen <ericvh@gmail.com>
Cc: Latchesar Ionkov <lucho@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

[PATCH 2/2] 9p: clear dangling pointers in p9stat_free

[Dominique Martinet]

From: Dominique Martinet <dominique.martinet@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.martinet@cea.fr>
Reported-by: syzbot+d4252148d198410b864f@syzkaller.appspotmail.com
Cc: Eric Van Hensbergen <ericvh@gmail.com>
Cc: Latchesar Ionkov <lucho@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