[PATCH] mm: don't clobber partially overlapping VMA with MAP_FIXED_NOREPLACE

[PATCH] mm: don't clobber partially overlapping VMA with MAP_FIXED_NOREPLACE

[Jann Horn]

Daniel Micay reports that attempting to use MAP_FIXED_NOREPLACE in an
application causes that application to randomly crash. The existing check
for handling MAP_FIXED_NOREPLACE looks up the first VMA that either
overlaps or follows the requested region, and then bails out if that VMA
overlaps *the start* of the requested region. It does not bail out if the
VMA only overlaps another part of the requested region.

Fix it by checking that the found VMA only starts at or after the end of
the requested region, in which case there is no overlap.

Reported-by: Daniel Micay <danielmicay@gmail.com>
Fixes: a4ff8e8620d3 ("mm: introduce MAP_FIXED_NOREPLACE")
Cc: stable@vger.kernel.org
Signed-off-by: Jann Horn <jannh@google.com>
---
 mm/mmap.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/mm/mmap.c b/mm/mmap.c
index 5f2b2b184c60..f7cd9cb966c0 100644
--- a/mm/mmap.c
+++ b/mm/mmap.c
@@ -1410,7 +1410,7 @@ unsigned long do_mmap(struct file *file, unsigned long addr,
 	if (flags & MAP_FIXED_NOREPLACE) {
 		struct vm_area_struct *vma = find_vma(mm, addr);
 
-		if (vma && vma->vm_start <= addr)
+		if (vma && vma->vm_start < addr + len)
 			return -EEXIST;
 	}
 
-- 
2.19.0.605.g01d371f741-goog

Re: [PATCH] mm: don't clobber partially overlapping VMA with MAP_FIXED_NOREPLACE

[Michal Hocko]

On Wed 10-10-18 17:27:36, Jann Horn wrote:
> Daniel Micay reports that attempting to use MAP_FIXED_NOREPLACE in an
> application causes that application to randomly crash. The existing check
> for handling MAP_FIXED_NOREPLACE looks up the first VMA that either
> overlaps or follows the requested region, and then bails out if that VMA
> overlaps *the start* of the requested region. It does not bail out if the
> VMA only overlaps another part of the requested region.

I do not understand. Could you give me an example?
[...]

> diff --git a/mm/mmap.c b/mm/mmap.c
> index 5f2b2b184c60..f7cd9cb966c0 100644
> --- a/mm/mmap.c
> +++ b/mm/mmap.c
> @@ -1410,7 +1410,7 @@ unsigned long do_mmap(struct file *file, unsigned long addr,
>  	if (flags & MAP_FIXED_NOREPLACE) {
>  		struct vm_area_struct *vma = find_vma(mm, addr);
>  
> -		if (vma && vma->vm_start <= addr)
> +		if (vma && vma->vm_start < addr + len)

find_vma is documented to - Look up the first VMA which satisfies addr <
vm_end, NULL if none.
This means that the above check guanratees that
	vm_start <= addr < vm_end
so an overlap is guanrateed. Why should we care how much we overlap?

-- 
Michal Hocko
SUSE Labs

Re: [PATCH] mm: don't clobber partially overlapping VMA with MAP_FIXED_NOREPLACE

[Jann Horn]

On Wed, Oct 10, 2018 at 7:19 PM Michal Hocko <mhocko@suse.com> wrote:
> On Wed 10-10-18 17:27:36, Jann Horn wrote:
> > Daniel Micay reports that attempting to use MAP_FIXED_NOREPLACE in an
> > application causes that application to randomly crash. The existing check
> > for handling MAP_FIXED_NOREPLACE looks up the first VMA that either
> > overlaps or follows the requested region, and then bails out if that VMA
> > overlaps *the start* of the requested region. It does not bail out if the
> > VMA only overlaps another part of the requested region.
>
> I do not understand. Could you give me an example?

Sure.

=======
user@debian:~$ cat mmap_fixed_simple.c
#include <sys/mman.h>
#include <errno.h>
#include <stdio.h>
#include <stdlib.h>
#include <unistd.h>

#ifndef MAP_FIXED_NOREPLACE
#define MAP_FIXED_NOREPLACE 0x100000
#endif

int main(void) {
  char *p;

  errno = 0;
  p = mmap((void*)0x10001000, 0x4000, PROT_NONE,
MAP_PRIVATE|MAP_ANONYMOUS|MAP_FIXED_NOREPLACE, -1, 0);
  printf("p1=%p err=%m\n", p);

  errno = 0;
  p = mmap((void*)0x10000000, 0x2000, PROT_READ,
MAP_PRIVATE|MAP_ANONYMOUS|MAP_FIXED_NOREPLACE, -1, 0);
  printf("p2=%p err=%m\n", p);

  char cmd[100];
  sprintf(cmd, "cat /proc/%d/maps", getpid());
  system(cmd);

  return 0;
}
user@debian:~$ gcc -o mmap_fixed_simple mmap_fixed_simple.c
user@debian:~$ ./mmap_fixed_simple
p1=0x10001000 err=Success
p2=0x10000000 err=Success
10000000-10002000 r--p 00000000 00:00 0
10002000-10005000 ---p 00000000 00:00 0
564a9a06f000-564a9a070000 r-xp 00000000 fe:01 264004
  /home/user/mmap_fixed_simple
564a9a26f000-564a9a270000 r--p 00000000 fe:01 264004
  /home/user/mmap_fixed_simple
564a9a270000-564a9a271000 rw-p 00001000 fe:01 264004
  /home/user/mmap_fixed_simple
564a9a54a000-564a9a56b000 rw-p 00000000 00:00 0                          [heap]
7f8eba447000-7f8eba5dc000 r-xp 00000000 fe:01 405885
  /lib/x86_64-linux-gnu/libc-2.24.so
7f8eba5dc000-7f8eba7dc000 ---p 00195000 fe:01 405885
  /lib/x86_64-linux-gnu/libc-2.24.so
7f8eba7dc000-7f8eba7e0000 r--p 00195000 fe:01 405885
  /lib/x86_64-linux-gnu/libc-2.24.so
7f8eba7e0000-7f8eba7e2000 rw-p 00199000 fe:01 405885
  /lib/x86_64-linux-gnu/libc-2.24.so
7f8eba7e2000-7f8eba7e6000 rw-p 00000000 00:00 0
7f8eba7e6000-7f8eba809000 r-xp 00000000 fe:01 405876
  /lib/x86_64-linux-gnu/ld-2.24.so
7f8eba9e9000-7f8eba9eb000 rw-p 00000000 00:00 0
7f8ebaa06000-7f8ebaa09000 rw-p 00000000 00:00 0
7f8ebaa09000-7f8ebaa0a000 r--p 00023000 fe:01 405876
  /lib/x86_64-linux-gnu/ld-2.24.so
7f8ebaa0a000-7f8ebaa0b000 rw-p 00024000 fe:01 405876
  /lib/x86_64-linux-gnu/ld-2.24.so
7f8ebaa0b000-7f8ebaa0c000 rw-p 00000000 00:00 0
7ffcc99fa000-7ffcc9a1b000 rw-p 00000000 00:00 0                          [stack]
7ffcc9b44000-7ffcc9b47000 r--p 00000000 00:00 0                          [vvar]
7ffcc9b47000-7ffcc9b49000 r-xp 00000000 00:00 0                          [vdso]
ffffffffff600000-ffffffffff601000 r-xp 00000000 00:00 0
  [vsyscall]
user@debian:~$ uname -a
Linux debian 4.19.0-rc6+ #181 SMP Wed Oct 3 23:43:42 CEST 2018 x86_64 GNU/Linux
user@debian:~$
=======

As you can see, the first page of the mapping at 0x10001000 was clobbered.

> > diff --git a/mm/mmap.c b/mm/mmap.c
> > index 5f2b2b184c60..f7cd9cb966c0 100644
> > --- a/mm/mmap.c
> > +++ b/mm/mmap.c
> > @@ -1410,7 +1410,7 @@ unsigned long do_mmap(struct file *file, unsigned long addr,
> >       if (flags & MAP_FIXED_NOREPLACE) {
> >               struct vm_area_struct *vma = find_vma(mm, addr);
> >
> > -             if (vma && vma->vm_start <= addr)
> > +             if (vma && vma->vm_start < addr + len)
>
> find_vma is documented to - Look up the first VMA which satisfies addr <
> vm_end, NULL if none.
> This means that the above check guanratees that
>         vm_start <= addr < vm_end
> so an overlap is guanrateed. Why should we care how much we overlap?

"an overlap is guaranteed"? I have no idea what you're trying to say.

Re: [PATCH] mm: don't clobber partially overlapping VMA with MAP_FIXED_NOREPLACE

[Michal Hocko]

On Wed 10-10-18 19:26:50, Jann Horn wrote:
[...]
> As you can see, the first page of the mapping at 0x10001000 was clobbered.
> 
> > > diff --git a/mm/mmap.c b/mm/mmap.c
> > > index 5f2b2b184c60..f7cd9cb966c0 100644
> > > --- a/mm/mmap.c
> > > +++ b/mm/mmap.c
> > > @@ -1410,7 +1410,7 @@ unsigned long do_mmap(struct file *file, unsigned long addr,
> > >       if (flags & MAP_FIXED_NOREPLACE) {
> > >               struct vm_area_struct *vma = find_vma(mm, addr);
> > >
> > > -             if (vma && vma->vm_start <= addr)
> > > +             if (vma && vma->vm_start < addr + len)
> >
> > find_vma is documented to - Look up the first VMA which satisfies addr <
> > vm_end, NULL if none.
> > This means that the above check guanratees that
> >         vm_start <= addr < vm_end
> > so an overlap is guanrateed. Why should we care how much we overlap?
> 
> "an overlap is guaranteed"? I have no idea what you're trying to say.

I have misread your changelog and the patch. Sorry about that. I thought
you meant a false possitive but you in fact meant false negative. Now it
makes complete sense.

Acked-by: Michal Hocko <mhocko@suse.com>

And thanks a lot for catching that!

-- 
Michal Hocko
SUSE Labs

Re: [PATCH] mm: don't clobber partially overlapping VMA with MAP_FIXED_NOREPLACE

[John Hubbard]

On 10/10/18 10:38 AM, Michal Hocko wrote:
> On Wed 10-10-18 19:26:50, Jann Horn wrote:
> [...]
>> As you can see, the first page of the mapping at 0x10001000 was clobbered.
>>
>>>> diff --git a/mm/mmap.c b/mm/mmap.c
>>>> index 5f2b2b184c60..f7cd9cb966c0 100644
>>>> --- a/mm/mmap.c
>>>> +++ b/mm/mmap.c
>>>> @@ -1410,7 +1410,7 @@ unsigned long do_mmap(struct file *file, unsigned long addr,
>>>>       if (flags & MAP_FIXED_NOREPLACE) {
>>>>               struct vm_area_struct *vma = find_vma(mm, addr);
>>>>
>>>> -             if (vma && vma->vm_start <= addr)
>>>> +             if (vma && vma->vm_start < addr + len)
>>>
>>> find_vma is documented to - Look up the first VMA which satisfies addr <
>>> vm_end, NULL if none.
>>> This means that the above check guanratees that
>>>         vm_start <= addr < vm_end
>>> so an overlap is guanrateed. Why should we care how much we overlap?
>>
>> "an overlap is guaranteed"? I have no idea what you're trying to say.
> 
> I have misread your changelog and the patch. Sorry about that. I thought
> you meant a false possitive but you in fact meant false negative. Now it
> makes complete sense.
> 
> Acked-by: Michal Hocko <mhocko@suse.com>
> 
> And thanks a lot for catching that!
> 

This also looks good to me. 

thanks,
-- 
John Hubbard
NVIDIA

Re: [PATCH] mm: don't clobber partially overlapping VMA with MAP_FIXED_NOREPLACE

[John Hubbard]

On 10/10/18 10:38 AM, Michal Hocko wrote:
> On Wed 10-10-18 19:26:50, Jann Horn wrote:
> [...]
>> As you can see, the first page of the mapping at 0x10001000 was clobbered.
>>
>>>> diff --git a/mm/mmap.c b/mm/mmap.c
>>>> index 5f2b2b184c60..f7cd9cb966c0 100644
>>>> --- a/mm/mmap.c
>>>> +++ b/mm/mmap.c
>>>> @@ -1410,7 +1410,7 @@ unsigned long do_mmap(struct file *file, unsigned long addr,
>>>>       if (flags & MAP_FIXED_NOREPLACE) {
>>>>               struct vm_area_struct *vma = find_vma(mm, addr);
>>>>
>>>> -             if (vma && vma->vm_start <= addr)
>>>> +             if (vma && vma->vm_start < addr + len)
>>>
>>> find_vma is documented to - Look up the first VMA which satisfies addr <
>>> vm_end, NULL if none.
>>> This means that the above check guanratees that
>>>         vm_start <= addr < vm_end
>>> so an overlap is guanrateed. Why should we care how much we overlap?
>>
>> "an overlap is guaranteed"? I have no idea what you're trying to say.
> 
> I have misread your changelog and the patch. Sorry about that. I thought
> you meant a false possitive but you in fact meant false negative. Now it
> makes complete sense.
> 
> Acked-by: Michal Hocko <mhocko@suse.com>
> 
> And thanks a lot for catching that!
> 

This also looks good to me. 

thanks,
-- 
John Hubbard
NVIDIA

Re: [PATCH] mm: don't clobber partially overlapping VMA with MAP_FIXED_NOREPLACE

[John Hubbard]

On 10/10/18 10:26 AM, Jann Horn wrote:
> On Wed, Oct 10, 2018 at 7:19 PM Michal Hocko <mhocko@suse.com> wrote:
>> On Wed 10-10-18 17:27:36, Jann Horn wrote:
>>> Daniel Micay reports that attempting to use MAP_FIXED_NOREPLACE in an
>>> application causes that application to randomly crash. The existing check
>>> for handling MAP_FIXED_NOREPLACE looks up the first VMA that either
>>> overlaps or follows the requested region, and then bails out if that VMA
>>> overlaps *the start* of the requested region. It does not bail out if the
>>> VMA only overlaps another part of the requested region.
>>
>> I do not understand. Could you give me an example?
> 
> Sure.
> 
> =======
> user@debian:~$ cat mmap_fixed_simple.c
> #include <sys/mman.h>
> #include <errno.h>
> #include <stdio.h>
> #include <stdlib.h>
> #include <unistd.h>
> 
> #ifndef MAP_FIXED_NOREPLACE
> #define MAP_FIXED_NOREPLACE 0x100000
> #endif
> 
> int main(void) {
>   char *p;
> 
>   errno = 0;
>   p = mmap((void*)0x10001000, 0x4000, PROT_NONE,
> MAP_PRIVATE|MAP_ANONYMOUS|MAP_FIXED_NOREPLACE, -1, 0);
>   printf("p1=%p err=%m\n", p);
> 
>   errno = 0;
>   p = mmap((void*)0x10000000, 0x2000, PROT_READ,
> MAP_PRIVATE|MAP_ANONYMOUS|MAP_FIXED_NOREPLACE, -1, 0);
>   printf("p2=%p err=%m\n", p);
> 
>   char cmd[100];
>   sprintf(cmd, "cat /proc/%d/maps", getpid());
>   system(cmd);
> 
>   return 0;
> }
> user@debian:~$ gcc -o mmap_fixed_simple mmap_fixed_simple.c
> user@debian:~$ ./mmap_fixed_simple
> p1=0x10001000 err=Success
> p2=0x10000000 err=Success
> 10000000-10002000 r--p 00000000 00:00 0
> 10002000-10005000 ---p 00000000 00:00 0
> 564a9a06f000-564a9a070000 r-xp 00000000 fe:01 264004
>   /home/user/mmap_fixed_simple
> 564a9a26f000-564a9a270000 r--p 00000000 fe:01 264004
>   /home/user/mmap_fixed_simple
> 564a9a270000-564a9a271000 rw-p 00001000 fe:01 264004
>   /home/user/mmap_fixed_simple
> 564a9a54a000-564a9a56b000 rw-p 00000000 00:00 0                          [heap]
> 7f8eba447000-7f8eba5dc000 r-xp 00000000 fe:01 405885
>   /lib/x86_64-linux-gnu/libc-2.24.so
> 7f8eba5dc000-7f8eba7dc000 ---p 00195000 fe:01 405885
>   /lib/x86_64-linux-gnu/libc-2.24.so
> 7f8eba7dc000-7f8eba7e0000 r--p 00195000 fe:01 405885
>   /lib/x86_64-linux-gnu/libc-2.24.so
> 7f8eba7e0000-7f8eba7e2000 rw-p 00199000 fe:01 405885
>   /lib/x86_64-linux-gnu/libc-2.24.so
> 7f8eba7e2000-7f8eba7e6000 rw-p 00000000 00:00 0
> 7f8eba7e6000-7f8eba809000 r-xp 00000000 fe:01 405876
>   /lib/x86_64-linux-gnu/ld-2.24.so
> 7f8eba9e9000-7f8eba9eb000 rw-p 00000000 00:00 0
> 7f8ebaa06000-7f8ebaa09000 rw-p 00000000 00:00 0
> 7f8ebaa09000-7f8ebaa0a000 r--p 00023000 fe:01 405876
>   /lib/x86_64-linux-gnu/ld-2.24.so
> 7f8ebaa0a000-7f8ebaa0b000 rw-p 00024000 fe:01 405876
>   /lib/x86_64-linux-gnu/ld-2.24.so
> 7f8ebaa0b000-7f8ebaa0c000 rw-p 00000000 00:00 0
> 7ffcc99fa000-7ffcc9a1b000 rw-p 00000000 00:00 0                          [stack]
> 7ffcc9b44000-7ffcc9b47000 r--p 00000000 00:00 0                          [vvar]
> 7ffcc9b47000-7ffcc9b49000 r-xp 00000000 00:00 0                          [vdso]
> ffffffffff600000-ffffffffff601000 r-xp 00000000 00:00 0
>   [vsyscall]
> user@debian:~$ uname -a
> Linux debian 4.19.0-rc6+ #181 SMP Wed Oct 3 23:43:42 CEST 2018 x86_64 GNU/Linux
> user@debian:~$
> =======
> 
> As you can see, the first page of the mapping at 0x10001000 was clobbered.

This looks good to me. The short example really helped, thanks for that.

(I think my first reply got bounced, sorry if this ends up as a duplicate email
for anyone.)

thanks,
-- 
John Hubbard
NVIDIA

Re: [PATCH] mm: don't clobber partially overlapping VMA with MAP_FIXED_NOREPLACE

[John Hubbard]

On 10/10/18 10:26 AM, Jann Horn wrote:
> On Wed, Oct 10, 2018 at 7:19 PM Michal Hocko <mhocko@suse.com> wrote:
>> On Wed 10-10-18 17:27:36, Jann Horn wrote:
>>> Daniel Micay reports that attempting to use MAP_FIXED_NOREPLACE in an
>>> application causes that application to randomly crash. The existing check
>>> for handling MAP_FIXED_NOREPLACE looks up the first VMA that either
>>> overlaps or follows the requested region, and then bails out if that VMA
>>> overlaps *the start* of the requested region. It does not bail out if the
>>> VMA only overlaps another part of the requested region.
>>
>> I do not understand. Could you give me an example?
> 
> Sure.
> 
> =======
> user@debian:~$ cat mmap_fixed_simple.c
> #include <sys/mman.h>
> #include <errno.h>
> #include <stdio.h>
> #include <stdlib.h>
> #include <unistd.h>
> 
> #ifndef MAP_FIXED_NOREPLACE
> #define MAP_FIXED_NOREPLACE 0x100000
> #endif
> 
> int main(void) {
>   char *p;
> 
>   errno = 0;
>   p = mmap((void*)0x10001000, 0x4000, PROT_NONE,
> MAP_PRIVATE|MAP_ANONYMOUS|MAP_FIXED_NOREPLACE, -1, 0);
>   printf("p1=%p err=%m\n", p);
> 
>   errno = 0;
>   p = mmap((void*)0x10000000, 0x2000, PROT_READ,
> MAP_PRIVATE|MAP_ANONYMOUS|MAP_FIXED_NOREPLACE, -1, 0);
>   printf("p2=%p err=%m\n", p);
> 
>   char cmd[100];
>   sprintf(cmd, "cat /proc/%d/maps", getpid());
>   system(cmd);
> 
>   return 0;
> }
> user@debian:~$ gcc -o mmap_fixed_simple mmap_fixed_simple.c
> user@debian:~$ ./mmap_fixed_simple
> p1=0x10001000 err=Success
> p2=0x10000000 err=Success
> 10000000-10002000 r--p 00000000 00:00 0
> 10002000-10005000 ---p 00000000 00:00 0
> 564a9a06f000-564a9a070000 r-xp 00000000 fe:01 264004
>   /home/user/mmap_fixed_simple
> 564a9a26f000-564a9a270000 r--p 00000000 fe:01 264004
>   /home/user/mmap_fixed_simple
> 564a9a270000-564a9a271000 rw-p 00001000 fe:01 264004
>   /home/user/mmap_fixed_simple
> 564a9a54a000-564a9a56b000 rw-p 00000000 00:00 0                          [heap]
> 7f8eba447000-7f8eba5dc000 r-xp 00000000 fe:01 405885
>   /lib/x86_64-linux-gnu/libc-2.24.so
> 7f8eba5dc000-7f8eba7dc000 ---p 00195000 fe:01 405885
>   /lib/x86_64-linux-gnu/libc-2.24.so
> 7f8eba7dc000-7f8eba7e0000 r--p 00195000 fe:01 405885
>   /lib/x86_64-linux-gnu/libc-2.24.so
> 7f8eba7e0000-7f8eba7e2000 rw-p 00199000 fe:01 405885
>   /lib/x86_64-linux-gnu/libc-2.24.so
> 7f8eba7e2000-7f8eba7e6000 rw-p 00000000 00:00 0
> 7f8eba7e6000-7f8eba809000 r-xp 00000000 fe:01 405876
>   /lib/x86_64-linux-gnu/ld-2.24.so
> 7f8eba9e9000-7f8eba9eb000 rw-p 00000000 00:00 0
> 7f8ebaa06000-7f8ebaa09000 rw-p 00000000 00:00 0
> 7f8ebaa09000-7f8ebaa0a000 r--p 00023000 fe:01 405876
>   /lib/x86_64-linux-gnu/ld-2.24.so
> 7f8ebaa0a000-7f8ebaa0b000 rw-p 00024000 fe:01 405876
>   /lib/x86_64-linux-gnu/ld-2.24.so
> 7f8ebaa0b000-7f8ebaa0c000 rw-p 00000000 00:00 0
> 7ffcc99fa000-7ffcc9a1b000 rw-p 00000000 00:00 0                          [stack]
> 7ffcc9b44000-7ffcc9b47000 r--p 00000000 00:00 0                          [vvar]
> 7ffcc9b47000-7ffcc9b49000 r-xp 00000000 00:00 0                          [vdso]
> ffffffffff600000-ffffffffff601000 r-xp 00000000 00:00 0
>   [vsyscall]
> user@debian:~$ uname -a
> Linux debian 4.19.0-rc6+ #181 SMP Wed Oct 3 23:43:42 CEST 2018 x86_64 GNU/Linux
> user@debian:~$
> =======
> 
> As you can see, the first page of the mapping at 0x10001000 was clobbered.

This looks good to me. The short example really helped, thanks for that.

(I think my first reply got bounced, sorry if this ends up as a duplicate email
for anyone.)

thanks,
-- 
John Hubbard
NVIDIA

Re: [PATCH] mm: don't clobber partially overlapping VMA with MAP_FIXED_NOREPLACE

[Michael Ellerman]

Jann Horn <jannh@google.com> writes:
> On Wed, Oct 10, 2018 at 7:19 PM Michal Hocko <mhocko@suse.com> wrote:
>> On Wed 10-10-18 17:27:36, Jann Horn wrote:
>> > Daniel Micay reports that attempting to use MAP_FIXED_NOREPLACE in an
>> > application causes that application to randomly crash. The existing check
>> > for handling MAP_FIXED_NOREPLACE looks up the first VMA that either
>> > overlaps or follows the requested region, and then bails out if that VMA
>> > overlaps *the start* of the requested region. It does not bail out if the
>> > VMA only overlaps another part of the requested region.
>>
>> I do not understand. Could you give me an example?
>
> Sure.
>
> =======
> user@debian:~$ cat mmap_fixed_simple.c
> #include <sys/mman.h>
> #include <errno.h>
> #include <stdio.h>
> #include <stdlib.h>
> #include <unistd.h>

..

Mind if I turn that into a selftest?

cheers

Re: [PATCH] mm: don't clobber partially overlapping VMA with MAP_FIXED_NOREPLACE

[Jann Horn]

On Fri, Oct 12, 2018 at 12:23 PM Michael Ellerman <mpe@ellerman.id.au> wrote:
> Jann Horn <jannh@google.com> writes:
> > On Wed, Oct 10, 2018 at 7:19 PM Michal Hocko <mhocko@suse.com> wrote:
> >> On Wed 10-10-18 17:27:36, Jann Horn wrote:
> >> > Daniel Micay reports that attempting to use MAP_FIXED_NOREPLACE in an
> >> > application causes that application to randomly crash. The existing check
> >> > for handling MAP_FIXED_NOREPLACE looks up the first VMA that either
> >> > overlaps or follows the requested region, and then bails out if that VMA
> >> > overlaps *the start* of the requested region. It does not bail out if the
> >> > VMA only overlaps another part of the requested region.
> >>
> >> I do not understand. Could you give me an example?
> >
> > Sure.
> >
> > =======
> > user@debian:~$ cat mmap_fixed_simple.c
> > #include <sys/mman.h>
> > #include <errno.h>
> > #include <stdio.h>
> > #include <stdlib.h>
> > #include <unistd.h>
>
> ..
>
> Mind if I turn that into a selftest?

Feel free to do that. :)

Re: [PATCH] mm: don't clobber partially overlapping VMA with MAP_FIXED_NOREPLACE

[Kees Cook]

On Wed, Oct 10, 2018 at 8:27 AM, Jann Horn <jannh@google.com> wrote:
> Daniel Micay reports that attempting to use MAP_FIXED_NOREPLACE in an
> application causes that application to randomly crash. The existing check
> for handling MAP_FIXED_NOREPLACE looks up the first VMA that either
> overlaps or follows the requested region, and then bails out if that VMA
> overlaps *the start* of the requested region. It does not bail out if the
> VMA only overlaps another part of the requested region.
>
> Fix it by checking that the found VMA only starts at or after the end of
> the requested region, in which case there is no overlap.
>
> Reported-by: Daniel Micay <danielmicay@gmail.com>
> Fixes: a4ff8e8620d3 ("mm: introduce MAP_FIXED_NOREPLACE")
> Cc: stable@vger.kernel.org
> Signed-off-by: Jann Horn <jannh@google.com>

Acked-by: Kees Cook <keescook@chromium.org>

Thanks for forwarding this!

Andrew, any chance we can get this into 4.19? (It'll end up in -stable
anyway, but it'd be nice to get it fixed now too.)

-Kees

> ---
>  mm/mmap.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/mm/mmap.c b/mm/mmap.c
> index 5f2b2b184c60..f7cd9cb966c0 100644
> --- a/mm/mmap.c
> +++ b/mm/mmap.c
> @@ -1410,7 +1410,7 @@ unsigned long do_mmap(struct file *file, unsigned long addr,
>         if (flags & MAP_FIXED_NOREPLACE) {
>                 struct vm_area_struct *vma = find_vma(mm, addr);
>
> -               if (vma && vma->vm_start <= addr)
> +               if (vma && vma->vm_start < addr + len)
>                         return -EEXIST;
>         }
>
> --
> 2.19.0.605.g01d371f741-goog
>



-- 
Kees Cook
Pixel Security

Re: [PATCH] mm: don't clobber partially overlapping VMA with MAP_FIXED_NOREPLACE

[Vlastimil Babka]

On 10/10/18 5:27 PM, Jann Horn wrote:
> Daniel Micay reports that attempting to use MAP_FIXED_NOREPLACE in an
> application causes that application to randomly crash. The existing check
> for handling MAP_FIXED_NOREPLACE looks up the first VMA that either
> overlaps or follows the requested region, and then bails out if that VMA
> overlaps *the start* of the requested region. It does not bail out if the
> VMA only overlaps another part of the requested region.
> 
> Fix it by checking that the found VMA only starts at or after the end of
> the requested region, in which case there is no overlap.
> 
> Reported-by: Daniel Micay <danielmicay@gmail.com>
> Fixes: a4ff8e8620d3 ("mm: introduce MAP_FIXED_NOREPLACE")
> Cc: stable@vger.kernel.org
> Signed-off-by: Jann Horn <jannh@google.com>

Good catch, thanks.

Acked-by: Vlastimil Babka <vbabka@suse.cz>

> ---
>  mm/mmap.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/mm/mmap.c b/mm/mmap.c
> index 5f2b2b184c60..f7cd9cb966c0 100644
> --- a/mm/mmap.c
> +++ b/mm/mmap.c
> @@ -1410,7 +1410,7 @@ unsigned long do_mmap(struct file *file, unsigned long addr,
>  	if (flags & MAP_FIXED_NOREPLACE) {
>  		struct vm_area_struct *vma = find_vma(mm, addr);
>  
> -		if (vma && vma->vm_start <= addr)
> +		if (vma && vma->vm_start < addr + len)
>  			return -EEXIST;
>  	}
>  
> 

Re: [PATCH] mm: don't clobber partially overlapping VMA with MAP_FIXED_NOREPLACE

[Khalid Aziz]

On 10/10/2018 09:27 AM, Jann Horn wrote:
> Daniel Micay reports that attempting to use MAP_FIXED_NOREPLACE in an
> application causes that application to randomly crash. The existing check
> for handling MAP_FIXED_NOREPLACE looks up the first VMA that either
> overlaps or follows the requested region, and then bails out if that VMA
> overlaps *the start* of the requested region. It does not bail out if the
> VMA only overlaps another part of the requested region.
> 
> Fix it by checking that the found VMA only starts at or after the end of
> the requested region, in which case there is no overlap.
> 
> Reported-by: Daniel Micay <danielmicay@gmail.com>
> Fixes: a4ff8e8620d3 ("mm: introduce MAP_FIXED_NOREPLACE")
> Cc: stable@vger.kernel.org
> Signed-off-by: Jann Horn <jannh@google.com>
> ---
>   mm/mmap.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/mm/mmap.c b/mm/mmap.c
> index 5f2b2b184c60..f7cd9cb966c0 100644
> --- a/mm/mmap.c
> +++ b/mm/mmap.c
> @@ -1410,7 +1410,7 @@ unsigned long do_mmap(struct file *file, unsigned long addr,
>   	if (flags & MAP_FIXED_NOREPLACE) {
>   		struct vm_area_struct *vma = find_vma(mm, addr);
>   
> -		if (vma && vma->vm_start <= addr)
> +		if (vma && vma->vm_start < addr + len)
>   			return -EEXIST;
>   	}
>   
> 

Makes sense.

Reviewed-by: Khalid Aziz <khalid.aziz@oracle.com>

--
Khalid