| Message ID | 20241206152032.1222067-2-bgeffon@google.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | mremap: Fix newaddr hint with MREMAP_DONTUNMAP | expand |
On Fri, Dec 6, 2024 at 12:20 PM Brian Geffon <bgeffon@google.com> wrote: > > Two non-mutually exclusive paths can land in mremap_to, MREMAP_FIXED > and MREMAP_DONTUNMAP which are called from mremap(). In the case of > MREMAP_FIXED we must validate the new_addr to ensure that the new > address is valid. In the case of MREMAP_DONTUNMAP without MREMAP_FIXED > a new address is specified as a hint, just like it would be in the > case of mmap. In this second case we don't need to perform any checks > because get_unmapped_area() will align new_addr, just like it would in > the case of mmap. > > Signed-off-by: Brian Geffon <bgeffon@google.com> > Reported-by: Marco Vanotti <mvanotti@google.com> > --- > mm/mremap.c | 26 +++++++++++++++++++------- > 1 file changed, 19 insertions(+), 7 deletions(-) > > diff --git a/mm/mremap.c b/mm/mremap.c > index 60473413836b..286ffdb883df 100644 > --- a/mm/mremap.c > +++ b/mm/mremap.c > @@ -912,15 +912,27 @@ static unsigned long mremap_to(unsigned long addr, unsigned long old_len, > unsigned long ret; > unsigned long map_flags = 0; > > - if (offset_in_page(new_addr)) > - return -EINVAL; > + /* > + * Two non-mutually exclusive paths can land in mremap_to, MREMAP_FIXED > + * and MREMAP_DONTUNMAP which are called from mremap(). In the case of > + * MREMAP_FIXED we must validate the new_addr to ensure that the new > + * address is valid. In the case of MREMAP_DONTUNMAP without MREMAP_FIXED > + * a new address is specified as a hint, just like it would be in the > + * case of mmap. In this second case we don't need to perform any checks > + * because get_unmapped_area() will align new_addr, just like it would in > + * the case of mmap. > + */ A few lines below we also check for MREMAP_FIXED before calling do_unmap, can't we do the validation there? > + if (flags & MREMAP_FIXED) { > + if (offset_in_page(new_addr)) > + return -EINVAL; > > - if (new_len > TASK_SIZE || new_addr > TASK_SIZE - new_len) > - return -EINVAL; > + if (new_len > TASK_SIZE || new_addr > TASK_SIZE - new_len) > + return -EINVAL; > > - /* Ensure the old/new locations do not overlap */ > - if (addr + old_len > new_addr && new_addr + new_len > addr) > - return -EINVAL; > + /* Ensure the old/new locations do not overlap */ > + if (addr + old_len > new_addr && new_addr + new_len > addr) > + return -EINVAL; > + } > > /* > * move_vma() need us to stay 4 maps below the threshold, otherwise > -- > 2.47.0.338.g60cca15819-goog >
On Fri, Dec 6, 2024 at 10:35 AM Marco Vanotti <mvanotti@google.com> wrote: > > On Fri, Dec 6, 2024 at 12:20 PM Brian Geffon <bgeffon@google.com> wrote: > > > > Two non-mutually exclusive paths can land in mremap_to, MREMAP_FIXED > > and MREMAP_DONTUNMAP which are called from mremap(). In the case of > > MREMAP_FIXED we must validate the new_addr to ensure that the new > > address is valid. In the case of MREMAP_DONTUNMAP without MREMAP_FIXED > > a new address is specified as a hint, just like it would be in the > > case of mmap. In this second case we don't need to perform any checks > > because get_unmapped_area() will align new_addr, just like it would in > > the case of mmap. > > > > Signed-off-by: Brian Geffon <bgeffon@google.com> > > Reported-by: Marco Vanotti <mvanotti@google.com> > > --- > > mm/mremap.c | 26 +++++++++++++++++++------- > > 1 file changed, 19 insertions(+), 7 deletions(-) > > > > diff --git a/mm/mremap.c b/mm/mremap.c > > index 60473413836b..286ffdb883df 100644 > > --- a/mm/mremap.c > > +++ b/mm/mremap.c > > @@ -912,15 +912,27 @@ static unsigned long mremap_to(unsigned long addr, unsigned long old_len, > > unsigned long ret; > > unsigned long map_flags = 0; > > > > - if (offset_in_page(new_addr)) > > - return -EINVAL; > > + /* > > + * Two non-mutually exclusive paths can land in mremap_to, MREMAP_FIXED > > + * and MREMAP_DONTUNMAP which are called from mremap(). In the case of > > + * MREMAP_FIXED we must validate the new_addr to ensure that the new > > + * address is valid. In the case of MREMAP_DONTUNMAP without MREMAP_FIXED > > + * a new address is specified as a hint, just like it would be in the > > + * case of mmap. In this second case we don't need to perform any checks > > + * because get_unmapped_area() will align new_addr, just like it would in > > + * the case of mmap. > > + */ > A few lines below we also check for MREMAP_FIXED before calling > do_unmap, can't we do the validation there? I don't have a strong preference either way. I can mail a v2 with this suggestion. > > + if (flags & MREMAP_FIXED) { > > + if (offset_in_page(new_addr)) > > + return -EINVAL; > > > > - if (new_len > TASK_SIZE || new_addr > TASK_SIZE - new_len) > > - return -EINVAL; > > + if (new_len > TASK_SIZE || new_addr > TASK_SIZE - new_len) > > + return -EINVAL; > > > > - /* Ensure the old/new locations do not overlap */ > > - if (addr + old_len > new_addr && new_addr + new_len > addr) > > - return -EINVAL; > > + /* Ensure the old/new locations do not overlap */ > > + if (addr + old_len > new_addr && new_addr + new_len > addr) > > + return -EINVAL; > > + } > > > > /* > > * move_vma() need us to stay 4 maps below the threshold, otherwise > > -- > > 2.47.0.338.g60cca15819-goog > >
diff --git a/mm/mremap.c b/mm/mremap.c index 60473413836b..286ffdb883df 100644 --- a/mm/mremap.c +++ b/mm/mremap.c @@ -912,15 +912,27 @@ static unsigned long mremap_to(unsigned long addr, unsigned long old_len, unsigned long ret; unsigned long map_flags = 0; - if (offset_in_page(new_addr)) - return -EINVAL; + /* + * Two non-mutually exclusive paths can land in mremap_to, MREMAP_FIXED + * and MREMAP_DONTUNMAP which are called from mremap(). In the case of + * MREMAP_FIXED we must validate the new_addr to ensure that the new + * address is valid. In the case of MREMAP_DONTUNMAP without MREMAP_FIXED + * a new address is specified as a hint, just like it would be in the + * case of mmap. In this second case we don't need to perform any checks + * because get_unmapped_area() will align new_addr, just like it would in + * the case of mmap. + */ + if (flags & MREMAP_FIXED) { + if (offset_in_page(new_addr)) + return -EINVAL; - if (new_len > TASK_SIZE || new_addr > TASK_SIZE - new_len) - return -EINVAL; + if (new_len > TASK_SIZE || new_addr > TASK_SIZE - new_len) + return -EINVAL; - /* Ensure the old/new locations do not overlap */ - if (addr + old_len > new_addr && new_addr + new_len > addr) - return -EINVAL; + /* Ensure the old/new locations do not overlap */ + if (addr + old_len > new_addr && new_addr + new_len > addr) + return -EINVAL; + } /* * move_vma() need us to stay 4 maps below the threshold, otherwise
Two non-mutually exclusive paths can land in mremap_to, MREMAP_FIXED and MREMAP_DONTUNMAP which are called from mremap(). In the case of MREMAP_FIXED we must validate the new_addr to ensure that the new address is valid. In the case of MREMAP_DONTUNMAP without MREMAP_FIXED a new address is specified as a hint, just like it would be in the case of mmap. In this second case we don't need to perform any checks because get_unmapped_area() will align new_addr, just like it would in the case of mmap. Signed-off-by: Brian Geffon <bgeffon@google.com> Reported-by: Marco Vanotti <mvanotti@google.com> --- mm/mremap.c | 26 +++++++++++++++++++------- 1 file changed, 19 insertions(+), 7 deletions(-)