| Message ID | 20201201132755.5076-1-liuzixian4@huawei.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | fix mmap return value when vma is merged after call_mmap() | expand |
On 01.12.20 14:27, Liu Zixian wrote: > On success, mmap should return the begin address of newly mapped area, > but patch "mm: mmap: merge vma after call_mmap() if possible" > set vm_start of newly merged vma to return value addr. > Users of mmap will get wrong address if vma is merged after call_mmap(). > We fix this by moving the assignment to addr before merging vma. > > Fixes: d70cec898324 ("mm: mmap: merge vma after call_mmap() if possible") > > Signed-off-by: Liu Zixian <liuzixian4@huawei.com> > --- > mm/mmap.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/mm/mmap.c b/mm/mmap.c > index d91ecb00d38c..9199b5e8cc1e 100644 > --- a/mm/mmap.c > +++ b/mm/mmap.c > @@ -1820,12 +1820,12 @@ unsigned long mmap_region(struct file *file, unsigned long addr, > * and cause general protection fault ultimately. > */ > fput(vma->vm_file); > - vm_area_free(vma); > - vma = merge; > /* Update vm_flags and possible addr to pick up the change. We don't > * warn here if addr changed as the vma is not linked by vma_link(). > */ > addr = vma->vm_start; > + vm_area_free(vma); > + vma = merge; > vm_flags = vma->vm_flags; > goto unmap_writable; > } > I assume this is quite hard to trigger, right (having vm_flags change)? "The vm_flags may be changed after call_mmap() because drivers may set some flags for their own purpose." Because how you describe it (returning wrong mmap address) this will completely mess up userspace.
On Tue, Dec 01, 2020 at 09:27:55PM +0800, Liu Zixian wrote: > On success, mmap should return the begin address of newly mapped area, > but patch "mm: mmap: merge vma after call_mmap() if possible" > set vm_start of newly merged vma to return value addr. > Users of mmap will get wrong address if vma is merged after call_mmap(). > We fix this by moving the assignment to addr before merging vma. > > Fixes: d70cec898324 ("mm: mmap: merge vma after call_mmap() if possible") > > Signed-off-by: Liu Zixian <liuzixian4@huawei.com> No blank line after Fixes: Consider a Cc: stable since this is quite a significant bug, if you hit it. > mm/mmap.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/mm/mmap.c b/mm/mmap.c > index d91ecb00d38c..9199b5e8cc1e 100644 > +++ b/mm/mmap.c > @@ -1820,12 +1820,12 @@ unsigned long mmap_region(struct file *file, unsigned long addr, > * and cause general protection fault ultimately. > */ > fput(vma->vm_file); > - vm_area_free(vma); > - vma = merge; > /* Update vm_flags and possible addr to pick up the change. We don't This comment is now out of date, we don't want to pick up the 'possible addr' change. > * warn here if addr changed as the vma is not linked by vma_link(). > */ > addr = vma->vm_start; > + vm_area_free(vma); > + vma = merge; > vm_flags = vma->vm_flags; > goto unmap_writable; > } David's remarks are correct, how was the original patch tested? Did it always prepend during merge so the new vm_start is OK? The previous patch really added more spaghetti to this function, jumping into the label 'unmap_writable' inside an if is aweful. At least for this fix it would be cleaner if the entire if(unlikely) block was moved below here: WARN_ON_ONCE(addr != vma->vm_start); addr = vma->vm_start; Since we want to do that unconditionally after call_mmap. Jason
Thanks for your review, I've sent a v2 patch.
diff --git a/mm/mmap.c b/mm/mmap.c index d91ecb00d38c..9199b5e8cc1e 100644 --- a/mm/mmap.c +++ b/mm/mmap.c @@ -1820,12 +1820,12 @@ unsigned long mmap_region(struct file *file, unsigned long addr, * and cause general protection fault ultimately. */ fput(vma->vm_file); - vm_area_free(vma); - vma = merge; /* Update vm_flags and possible addr to pick up the change. We don't * warn here if addr changed as the vma is not linked by vma_link(). */ addr = vma->vm_start; + vm_area_free(vma); + vma = merge; vm_flags = vma->vm_flags; goto unmap_writable; }
On success, mmap should return the begin address of newly mapped area, but patch "mm: mmap: merge vma after call_mmap() if possible" set vm_start of newly merged vma to return value addr. Users of mmap will get wrong address if vma is merged after call_mmap(). We fix this by moving the assignment to addr before merging vma. Fixes: d70cec898324 ("mm: mmap: merge vma after call_mmap() if possible") Signed-off-by: Liu Zixian <liuzixian4@huawei.com> --- mm/mmap.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)