On Thu, 2 Jan 2014 13:10:28 -0800 Linus Torvalds <torvalds@xxxxxxxxxxxxxxxxxxxx> wrote:
> On Thu, Jan 2, 2014 at 12:58 PM, <akpm@xxxxxxxxxxxxxxxxxxxx> wrote:
> > From: Rik van Riel <riel@xxxxxxxxxx>
> > Subject: mm: fix use-after-free in sys_remap_file_pages
> >
> > remap_file_pages calls mmap_region, which may merge the VMA with other
> > existing VMAs, and free "vma". This can lead to a use-after-free bug.
> > Avoid the bug by remembering vm_flags before calling mmap_region, and not
> > trying to dereference vma later.
> >
> >
> > diff -puN mm/fremap.c~mm-fix-use-after-free-in-sys_remap_file_pages mm/fremap.c
> > --- a/mm/fremap.c~mm-fix-use-after-free-in-sys_remap_file_pages
> > +++ a/mm/fremap.c
> > @@ -208,9 +208,10 @@ get_write_lock:
> > if (mapping_cap_account_dirty(mapping)) {
> > unsigned long addr;
> > struct file *file = get_file(vma->vm_file);
> > + /* mmap_region may free vma; grab the info now */
> > + vm_flags = ACCESS_ONCE(vma->vm_flags);
> >
> > - addr = mmap_region(file, start, size,
> > - vma->vm_flags, pgoff);
> > + addr = mmap_region(file, start, size, vm_flags, pgoff);
>
> What's the logic begind the ACCESS_ONCE() part? That makes no sense to
> me. Sure, mmap_region() may free the vma, and I agree with the patch
> per se, but the ACCESS_ONCE() looks like some random voodoo
> programming to me..
Yes, it's unneeded. afaict this was done for documentation/clarity
reasons - to make it clear(er) that *vma is about to die. But that
isn't what ACCESS_ONCE is for and the code comment does a fine job of
this.
I'll queue a patch to remove it.
--
To unsubscribe from this list: send the line "unsubscribe stable" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html