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

["Kirill A. Shutemov" <kirill@shutemov.name>]

On Mon, Sep 09, 2019 at 06:55:21AM -0700, Matthew Wilcox wrote:
> 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?

Are you talking about maybe_unlock_mmap_for_io()? Yeah, worth moving it to
mm/internal.h and reuse.

Care to prepare the patch? :P

-- 
 Kirill A. Shutemov