[PATCH] linux-user/elfload: do not assume MAP_FIXED_NOREPLACE kernel support

[PATCH] linux-user/elfload: do not assume MAP_FIXED_NOREPLACE kernel support

[Vincent Fazio]

From: Vincent Fazio <vfazio@gmail.com>

Previously, pgd_find_hole_fallback assumed that if the build host's libc
had MAP_FIXED_NOREPLACE defined that the address returned by mmap would
match the requested address. This is not a safe assumption for Linux
kernels prior to 4.17

Now, we always compare mmap's resultant address with the requested
address and no longer short-circuit based on MAP_FIXED_NOREPLACE.

Fixes: 2667e069e7b5 ("linux-user: don't use MAP_FIXED in pgd_find_hole_fallback")
Signed-off-by: Vincent Fazio <vfazio@gmail.com>
---
 linux-user/elfload.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/linux-user/elfload.c b/linux-user/elfload.c
index 5f5f23d2e5..8d425f9ed0 100644
--- a/linux-user/elfload.c
+++ b/linux-user/elfload.c
@@ -2217,8 +2217,7 @@ static uintptr_t pgd_find_hole_fallback(uintptr_t guest_size, uintptr_t brk,
                                      PROT_NONE, flags, -1, 0);
             if (mmap_start != MAP_FAILED) {
                 munmap(mmap_start, guest_size);
-                if (MAP_FIXED_NOREPLACE != 0 ||
-                    mmap_start == (void *) align_start) {
+                if (mmap_start == (void *) align_start) {
                     return (uintptr_t) mmap_start + offset;
                 }
             }
-- 
2.30.0

Re: [PATCH] linux-user/elfload: do not assume MAP_FIXED_NOREPLACE kernel support

[Laurent Vivier]

Le 31/01/2021 à 07:19, Vincent Fazio a écrit :
> From: Vincent Fazio <vfazio@gmail.com>
> 
> Previously, pgd_find_hole_fallback assumed that if the build host's libc
> had MAP_FIXED_NOREPLACE defined that the address returned by mmap would
> match the requested address. This is not a safe assumption for Linux
> kernels prior to 4.17
> 
> Now, we always compare mmap's resultant address with the requested
> address and no longer short-circuit based on MAP_FIXED_NOREPLACE.
> 
> Fixes: 2667e069e7b5 ("linux-user: don't use MAP_FIXED in pgd_find_hole_fallback")
> Signed-off-by: Vincent Fazio <vfazio@gmail.com>
> ---
>  linux-user/elfload.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/linux-user/elfload.c b/linux-user/elfload.c
> index 5f5f23d2e5..8d425f9ed0 100644
> --- a/linux-user/elfload.c
> +++ b/linux-user/elfload.c
> @@ -2217,8 +2217,7 @@ static uintptr_t pgd_find_hole_fallback(uintptr_t guest_size, uintptr_t brk,
>                                       PROT_NONE, flags, -1, 0);
>              if (mmap_start != MAP_FAILED) {
>                  munmap(mmap_start, guest_size);
> -                if (MAP_FIXED_NOREPLACE != 0 ||
> -                    mmap_start == (void *) align_start) {
> +                if (mmap_start == (void *) align_start) {
>                      return (uintptr_t) mmap_start + offset;
>                  }
>              }
> 

Reviewed-by: Laurent Vivier <laurent@vivier.eu>

CC: Alex

Why did you put this checking in first place?

Thanks,
Laurent

Re: [PATCH] linux-user/elfload: do not assume MAP_FIXED_NOREPLACE kernel support

[Alex Bennée]

Vincent Fazio <vfazio@xes-inc.com> writes:

> From: Vincent Fazio <vfazio@gmail.com>
>
> Previously, pgd_find_hole_fallback assumed that if the build host's libc
> had MAP_FIXED_NOREPLACE defined that the address returned by mmap would
> match the requested address. This is not a safe assumption for Linux
> kernels prior to 4.17

It doesn't as we have in osdep.h:

  #ifndef MAP_FIXED_NOREPLACE
  #define MAP_FIXED_NOREPLACE 0
  #endif

which is to say to assume if MAP_FIXED_NOREPLACE is defined the kernel
should have given us what we want otherwise we do the check.

>
> Now, we always compare mmap's resultant address with the requested
> address and no longer short-circuit based on MAP_FIXED_NOREPLACE.
>
> Fixes: 2667e069e7b5 ("linux-user: don't use MAP_FIXED in pgd_find_hole_fallback")
> Signed-off-by: Vincent Fazio <vfazio@gmail.com>
> ---
>  linux-user/elfload.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/linux-user/elfload.c b/linux-user/elfload.c
> index 5f5f23d2e5..8d425f9ed0 100644
> --- a/linux-user/elfload.c
> +++ b/linux-user/elfload.c
> @@ -2217,8 +2217,7 @@ static uintptr_t pgd_find_hole_fallback(uintptr_t guest_size, uintptr_t brk,
>                                       PROT_NONE, flags, -1, 0);
>              if (mmap_start != MAP_FAILED) {
>                  munmap(mmap_start, guest_size);
> -                if (MAP_FIXED_NOREPLACE != 0 ||
> -                    mmap_start == (void *) align_start) {
> +                if (mmap_start == (void *) align_start) {
>                      return (uintptr_t) mmap_start + offset;
>                  }
>              }


-- 
Alex Bennée

Re: [PATCH] linux-user/elfload: do not assume MAP_FIXED_NOREPLACE kernel support

[Laurent Vivier]

Le 14/02/2021 à 12:24, Alex Bennée a écrit :
> 
> Vincent Fazio <vfazio@xes-inc.com> writes:
> 
>> From: Vincent Fazio <vfazio@gmail.com>
>>
>> Previously, pgd_find_hole_fallback assumed that if the build host's libc
>> had MAP_FIXED_NOREPLACE defined that the address returned by mmap would
>> match the requested address. This is not a safe assumption for Linux
>> kernels prior to 4.17
> 
> It doesn't as we have in osdep.h:
> 
>   #ifndef MAP_FIXED_NOREPLACE
>   #define MAP_FIXED_NOREPLACE 0
>   #endif
> 
> which is to say to assume if MAP_FIXED_NOREPLACE is defined the kernel
> should have given us what we want otherwise we do the check.


But what is the purpose of the "if (MAP_FIXED_NOREPLACE != 0 ||"?
Can't we rely only on "mmap_start == (void *) align_start"?

Thanks,
Laurent

>>
>> Now, we always compare mmap's resultant address with the requested
>> address and no longer short-circuit based on MAP_FIXED_NOREPLACE.
>>
>> Fixes: 2667e069e7b5 ("linux-user: don't use MAP_FIXED in pgd_find_hole_fallback")
>> Signed-off-by: Vincent Fazio <vfazio@gmail.com>
>> ---
>>  linux-user/elfload.c | 3 +--
>>  1 file changed, 1 insertion(+), 2 deletions(-)
>>
>> diff --git a/linux-user/elfload.c b/linux-user/elfload.c
>> index 5f5f23d2e5..8d425f9ed0 100644
>> --- a/linux-user/elfload.c
>> +++ b/linux-user/elfload.c
>> @@ -2217,8 +2217,7 @@ static uintptr_t pgd_find_hole_fallback(uintptr_t guest_size, uintptr_t brk,
>>                                       PROT_NONE, flags, -1, 0);
>>              if (mmap_start != MAP_FAILED) {
>>                  munmap(mmap_start, guest_size);
>> -                if (MAP_FIXED_NOREPLACE != 0 ||
>> -                    mmap_start == (void *) align_start) {
>> +                if (mmap_start == (void *) align_start) {
>>                      return (uintptr_t) mmap_start + offset;
>>                  }
>>              }
> 
> 

Re: [PATCH] linux-user/elfload: do not assume MAP_FIXED_NOREPLACE kernel support

[Vincent Fazio]

On Sun, Feb 14, 2021 at 6:50 AM Laurent Vivier <laurent@vivier.eu> wrote:
>
> Le 14/02/2021 à 12:24, Alex Bennée a écrit :
> >
> > Vincent Fazio <vfazio@xes-inc.com> writes:
> >
> >> From: Vincent Fazio <vfazio@gmail.com>
> >>
> >> Previously, pgd_find_hole_fallback assumed that if the build host's libc
> >> had MAP_FIXED_NOREPLACE defined that the address returned by mmap would
> >> match the requested address. This is not a safe assumption for Linux
> >> kernels prior to 4.17
> >
> > It doesn't as we have in osdep.h:
> >
> >   #ifndef MAP_FIXED_NOREPLACE
> >   #define MAP_FIXED_NOREPLACE 0
> >   #endif
> >
> > which is to say to assume if MAP_FIXED_NOREPLACE is defined the kernel
> > should have given us what we want otherwise we do the check.
>
>
> But what is the purpose of the "if (MAP_FIXED_NOREPLACE != 0 ||"?
> Can't we rely only on "mmap_start == (void *) align_start"?
>
> Thanks,
> Laurent
>

I think we have to rely on address matching. The problem is
specifically when MAP_FIXED_NOREPLACE is defined and is passed to mmap
but the running kernel doesn't know what to do with the flag so
returns a value that is not what was hinted at. Previously the code
assumed that if MAP_FIXED_NOREPLACE was defined that the returned
address would match, but that isn't always the case if the kernel
doesn't have support for the flag. The 4.4, 4.9 and 4.14 LTS kernels
are still in use and could run into this problem.

-Vincent

Re: [PATCH] linux-user/elfload: do not assume MAP_FIXED_NOREPLACE kernel support

[Alex Bennée]

Vincent Fazio <vfazio@gmail.com> writes:

> On Sun, Feb 14, 2021 at 6:50 AM Laurent Vivier <laurent@vivier.eu> wrote:
>>
>> Le 14/02/2021 à 12:24, Alex Bennée a écrit :
>> >
>> > Vincent Fazio <vfazio@xes-inc.com> writes:
>> >
>> >> From: Vincent Fazio <vfazio@gmail.com>
>> >>
>> >> Previously, pgd_find_hole_fallback assumed that if the build host's libc
>> >> had MAP_FIXED_NOREPLACE defined that the address returned by mmap would
>> >> match the requested address. This is not a safe assumption for Linux
>> >> kernels prior to 4.17
>> >
>> > It doesn't as we have in osdep.h:
>> >
>> >   #ifndef MAP_FIXED_NOREPLACE
>> >   #define MAP_FIXED_NOREPLACE 0
>> >   #endif
>> >
>> > which is to say to assume if MAP_FIXED_NOREPLACE is defined the kernel
>> > should have given us what we want otherwise we do the check.
>>
>>
>> But what is the purpose of the "if (MAP_FIXED_NOREPLACE != 0 ||"?
>> Can't we rely only on "mmap_start == (void *) align_start"?
>>
>> Thanks,
>> Laurent
>>
>
> I think we have to rely on address matching. The problem is
> specifically when MAP_FIXED_NOREPLACE is defined and is passed to mmap
> but the running kernel doesn't know what to do with the flag so
> returns a value that is not what was hinted at. Previously the code
> assumed that if MAP_FIXED_NOREPLACE was defined that the returned
> address would match, but that isn't always the case if the kernel
> doesn't have support for the flag. The 4.4, 4.9 and 4.14 LTS kernels
> are still in use and could run into this problem.

Ahh right so I think this is a case of binaries being built on a
different platform than kernel they are running on. In which case the
flag would be defined but the underlying kernel fails to identify it. Is
this a container like case by any chance?

If I'd read the man page closer:

   Note   that   older   kernels   which   do   not  recognize  the
   MAP_FIXED_NOREPLACE flag will typically (upon detecting a colli‐
   sion  with a preexisting mapping) fall back to a "non-MAP_FIXED"
   type of behavior: they will return an address that is  different
   from  the  requested  address.   Therefore,  backward-compatible
   software should check the returned address against the requested
   address.

so yes we should avoid short circuiting the return address check.

Reviewed-by: Alex Bennée <alex.bennee@linaro.org>

-- 
Alex Bennée

Re: [PATCH] linux-user/elfload: do not assume MAP_FIXED_NOREPLACE kernel support

[Vincent Fazio]

On 2/15/21 3:52 AM, Alex Bennée wrote:
> 
> Vincent Fazio <vfazio@gmail.com> writes:
> 
> 
> Ahh right so I think this is a case of binaries being built on a
> different platform than kernel they are running on. In which case the
> flag would be defined but the underlying kernel fails to identify it. Is
> this a container like case by any chance?
Yes, my builds were happening in a container to eventually have the statically built binaries run in another container. 
I discovered this issue (and the two others reviewed) while trying to debootstrap Debian Bullseye in a container.
> 
> If I'd read the man page closer:
> 
>     Note   that   older   kernels   which   do   not  recognize  the
>     MAP_FIXED_NOREPLACE flag will typically (upon detecting a colli‐
>     sion  with a preexisting mapping) fall back to a "non-MAP_FIXED"
>     type of behavior: they will return an address that is  different
>     from  the  requested  address.   Therefore,  backward-compatible
>     software should check the returned address against the requested
>     address.
> 
> so yes we should avoid short circuiting the return address check.
> 
> Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
> 

Re: [PATCH] linux-user/elfload: do not assume MAP_FIXED_NOREPLACE kernel support

[Laurent Vivier]

Le 31/01/2021 à 07:19, Vincent Fazio a écrit :
> From: Vincent Fazio <vfazio@gmail.com>
> 
> Previously, pgd_find_hole_fallback assumed that if the build host's libc
> had MAP_FIXED_NOREPLACE defined that the address returned by mmap would
> match the requested address. This is not a safe assumption for Linux
> kernels prior to 4.17
> 
> Now, we always compare mmap's resultant address with the requested
> address and no longer short-circuit based on MAP_FIXED_NOREPLACE.
> 
> Fixes: 2667e069e7b5 ("linux-user: don't use MAP_FIXED in pgd_find_hole_fallback")
> Signed-off-by: Vincent Fazio <vfazio@gmail.com>
> ---
>  linux-user/elfload.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/linux-user/elfload.c b/linux-user/elfload.c
> index 5f5f23d2e5..8d425f9ed0 100644
> --- a/linux-user/elfload.c
> +++ b/linux-user/elfload.c
> @@ -2217,8 +2217,7 @@ static uintptr_t pgd_find_hole_fallback(uintptr_t guest_size, uintptr_t brk,
>                                       PROT_NONE, flags, -1, 0);
>              if (mmap_start != MAP_FAILED) {
>                  munmap(mmap_start, guest_size);
> -                if (MAP_FIXED_NOREPLACE != 0 ||
> -                    mmap_start == (void *) align_start) {
> +                if (mmap_start == (void *) align_start) {
>                      return (uintptr_t) mmap_start + offset;
>                  }
>              }
> 

Applied to my linux-user-for-6.0 branch.

Thanks,
Laurent