linux-kernel - Re: KASAN: use-after-free Read in shmem

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20190909135521.GD29434@bombadil.infradead.org>
Date:   Mon, 9 Sep 2019 06:55:21 -0700
From:   Matthew Wilcox <willy@...radead.org>
To:     "Kirill A. Shutemov" <kirill@...temov.name>
Cc:     Hillf Danton <hdanton@...a.com>,
        syzbot <syzbot+03ee87124ee05af991bd@...kaller.appspotmail.com>,
        hughd@...gle.com, linux-kernel@...r.kernel.org, linux-mm@...ck.org,
        syzkaller-bugs@...glegroups.com
Subject: Re: KASAN: use-after-free Read in shmem_fault (2)

On Mon, Sep 02, 2019 at 05:20:30PM +0300, Kirill A. Shutemov wrote:
> On Mon, Sep 02, 2019 at 06:52:54AM -0700, Matthew Wilcox wrote:
> > On Sat, Aug 31, 2019 at 12:58:26PM +0800, Hillf Danton wrote:
> > > On Fri, 30 Aug 2019 12:40:06 -0700
> > > > syzbot found the following crash on:
> > > > 
> > > > HEAD commit:    a55aa89a Linux 5.3-rc6
> > > > git tree:       upstream
> > > > console output: https://syzkaller.appspot.com/x/log.txt?x=12f4beb6600000
> > > > kernel config:  https://syzkaller.appspot.com/x/.config?x=2a6a2b9826fdadf9
> > > > dashboard link: https://syzkaller.appspot.com/bug?extid=03ee87124ee05af991bd
> > > > compiler:       gcc (GCC) 9.0.0 20181231 (experimental)
> > > > 
> > > > ==================================================================
> > > > BUG: KASAN: use-after-free in perf_trace_lock_acquire+0x401/0x530  
> > > > include/trace/events/lock.h:13
> > > > Read of size 8 at addr ffff8880a5cf2c50 by task syz-executor.0/26173
> > > 
> > > --- a/mm/shmem.c
> > > +++ b/mm/shmem.c
> > > @@ -2021,6 +2021,12 @@ static vm_fault_t shmem_fault(struct vm_
> > >  			shmem_falloc_waitq = shmem_falloc->waitq;
> > >  			prepare_to_wait(shmem_falloc_waitq, &shmem_fault_wait,
> > >  					TASK_UNINTERRUPTIBLE);
> > > +			/*
> > > +			 * it is not trivial to see what will take place after
> > > +			 * releasing i_lock and taking a nap, so hold inode to
> > > +			 * be on the safe side.
> > 
> > I think the comment could be improved.  How about:
> > 
> > 			 * The file could be unmapped by another thread after
> > 			 * releasing i_lock, and the inode then freed.  Hold
> > 			 * a reference to the inode to prevent this.
> 
> It only can happen if mmap_sem was released, so it's better to put
> __iget() to the branch above next to up_read(). I've got confused at first
> how it is possible from ->fault().
> 
> This way iput() below should only be called for ret == VM_FAULT_RETRY.

Looking at the rather similar construct in filemap.c, should we solve
it the same way, where we inc the refcount on the struct file instead
of the inode before releasing the mmap_sem?