lkml.org 
[lkml]   [2019]   [Sep]   [9]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: KASAN: use-after-free Read in shmem_fault (2)
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

\
 
 \ /
  Last update: 2019-09-09 17:05    [W:0.066 / U:0.188 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site