Re: KASAN: use-after-free Read in shmem_fault (2)

[Matthew Wilcox]

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?