Messages in this thread Patch in this message | | | From | "PaX Team" <> | Date | Fri, 13 Dec 2013 12:10:17 +0100 | Subject | Re: [PATCH] mm: fix use-after-free in sys_remap_file_pages |
| |
On 12 Dec 2013 at 22:41, Rik van Riel wrote:
> If the vma has been freed by the time the code jumps to the > out label (because it was freed by a function called from > mmap_region), surely it will also already have been freed > by the time this patch dereferences it?
oops, yes, i meant to save the flags away before mmap_region, no idea how i ended up with this ;).
> Also, setting vma = NULL to avoid the if (vma) branch at > the out: label is unnecessarily obfuscated. Lets make things > clear by documenting what is going on, and having a label > after that dereference.
on that note, how about this as well:
> --- a/mm/fremap.c > +++ b/mm/fremap.c > @@ -203,6 +203,7 @@ SYSCALL_DEFINE5(remap_file_pages, unsigned long, start, unsigned long, size, > if (mapping_cap_account_dirty(mapping)) { > unsigned long addr; > struct file *file = get_file(vma->vm_file); > + vm_flags = vma->vm_flags; > > addr = mmap_region(file, start, size, > vma->vm_flags, pgoff); ^^^^^^^^^^^^^ pass in vm_flags instead of vma->vm_flags just to prevent someone from 'optimizing' away the read in the future?
> @@ -213,7 +214,8 @@ SYSCALL_DEFINE5(remap_file_pages, unsigned long, start, unsigned long, size, > BUG_ON(addr != start); > err = 0; > } > - goto out; > + /* mmap_region may have freed vma */ > + goto out_freed;
perhaps {copy,move} this comment above the previous hunk since that's where the relevant action is?
cheers, PaX Team
| |