A user is not required to set a new address when using
MREMAP_DONTUNMAP as it can be used without MREMAP_FIXED.
When doing so the remap event will use new_addr which may not
have been set and we didn't propagate it back other then
in the return value of remap_to.
Because ret is always the new address it's probably more
correct to use it rather than new_addr on the remap_event_complete
call, and it resolves this bug.
Signed-off-by: Brian Geffon <[email protected]>
---
mm/mremap.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/mm/mremap.c b/mm/mremap.c
index c881abeba0bf..6aa6ea605068 100644
--- a/mm/mremap.c
+++ b/mm/mremap.c
@@ -794,7 +794,7 @@ SYSCALL_DEFINE5(mremap, unsigned long, addr, unsigned long, old_len,
if (locked && new_len > old_len)
mm_populate(new_addr + old_len, new_len - old_len);
userfaultfd_unmap_complete(mm, &uf_unmap_early);
- mremap_userfaultfd_complete(&uf, addr, new_addr, old_len);
+ mremap_userfaultfd_complete(&uf, addr, ret, old_len);
userfaultfd_unmap_complete(mm, &uf_unmap);
return ret;
}
--
2.26.2.526.g744177e7f7-goog
On Wed, May 6, 2020 at 1:22 PM Brian Geffon <[email protected]> wrote:
>
> A user is not required to set a new address when using
> MREMAP_DONTUNMAP as it can be used without MREMAP_FIXED.
> When doing so the remap event will use new_addr which may not
> have been set and we didn't propagate it back other then
> in the return value of remap_to.
>
> Because ret is always the new address it's probably more
> correct to use it rather than new_addr on the remap_event_complete
> call, and it resolves this bug.
>
> Signed-off-by: Brian Geffon <[email protected]>
> ---
> mm/mremap.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/mm/mremap.c b/mm/mremap.c
> index c881abeba0bf..6aa6ea605068 100644
> --- a/mm/mremap.c
> +++ b/mm/mremap.c
> @@ -794,7 +794,7 @@ SYSCALL_DEFINE5(mremap, unsigned long, addr, unsigned long, old_len,
> if (locked && new_len > old_len)
> mm_populate(new_addr + old_len, new_len - old_len);
> userfaultfd_unmap_complete(mm, &uf_unmap_early);
> - mremap_userfaultfd_complete(&uf, addr, new_addr, old_len);
> + mremap_userfaultfd_complete(&uf, addr, ret, old_len);
Not super familiar with this code, but thought I'd ask, does ret need
to be checked for -ENOMEM before calling mremap_userfaultfd_complete?
Sorry if I missed something.
Thanks,
- Joel
> userfaultfd_unmap_complete(mm, &uf_unmap);
> return ret;
> }
> --
> 2.26.2.526.g744177e7f7-goog
>
> > - mremap_userfaultfd_complete(&uf, addr, new_addr, old_len);
> > + mremap_userfaultfd_complete(&uf, addr, ret, old_len);
>
> Not super familiar with this code, but thought I'd ask, does ret
> to be checked for -ENOMEM before calling mremap_userfaultfd_complete?
> Sorry if I missed something.
No, mremap_userfaultfd_complete will do a check similar to
offset_in_page() by checking the page mask.
It does (to & ~PAGE_MASK) to check for a non-aligned "to" value, so we're good.
Additionally, earlier in the process then ctx will be null because we
will have never called mremap_userfaultfd_prep,
and mremap_userfaultfd_complete will check if there is a context
before proceeding.
Brian
On Wed, May 6, 2020 at 9:11 PM Brian Geffon <[email protected]> wrote:
>
> > > - mremap_userfaultfd_complete(&uf, addr, new_addr, old_len);
> > > + mremap_userfaultfd_complete(&uf, addr, ret, old_len);
> >
> > Not super familiar with this code, but thought I'd ask, does ret
> > to be checked for -ENOMEM before calling mremap_userfaultfd_complete?
> > Sorry if I missed something.
>
> No, mremap_userfaultfd_complete will do a check similar to
> offset_in_page() by checking the page mask.
> It does (to & ~PAGE_MASK) to check for a non-aligned "to" value, so we're good.
>
> Additionally, earlier in the process then ctx will be null because we
> will have never called mremap_userfaultfd_prep,
> and mremap_userfaultfd_complete will check if there is a context
> before proceeding.
Makes sense.
thanks,
- Joel