Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
In-Reply-To: <20180827052412.GA26294@nautica>
References: <000000000000af648b057456e234@google.com> <20180827052412.GA26294@nautica>
From:   Dmitry Vyukov <dvyukov@google.com>
Date:   Mon, 27 Aug 2018 07:25:18 -0700
Message-ID: <CACT4Y+b1B5_Eg+j91GxaVbPk5sb8+SVOU_z5k9HYb4R716X9kQ@mail.gmail.com>
Subject: Re: KASAN: invalid-free in p9stat_free
To:     Dominique Martinet <asmadeus@codewreck.org>
Cc:     syzbot <syzbot+d4252148d198410b864f@syzkaller.appspotmail.com>,
        David Miller <davem@davemloft.net>,
        Eric Van Hensbergen <ericvh@gmail.com>,
        LKML <linux-kernel@vger.kernel.org>,
        Latchesar Ionkov <lucho@ionkov.net>,
        netdev <netdev@vger.kernel.org>,
        syzkaller-bugs <syzkaller-bugs@googlegroups.com>,
        v9fs-developer@lists.sourceforge.net
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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.