Re: KASAN: invalid-free in p9stat_free

On Sun, Aug 26, 2018 at 10:24 PM, Dominique Martinet
[off-list ref] wrote:

syzbot wrote on Sun, Aug 26, 2018:

quoted

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.