[PATCH] linux-user: Ensure mmap_min_addr is non-zero

Richard Henderson posted 1 patch 3 years, 8 months ago
Test docker-quick@centos7 failed
Test docker-mingw@fedora failed
Test checkpatch failed
Test FreeBSD failed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20200724212314.545877-1-richard.henderson@linaro.org
Maintainers: Laurent Vivier <laurent@vivier.eu>
linux-user/main.c | 16 ++++++++++++++--
1 file changed, 14 insertions(+), 2 deletions(-)
[PATCH] linux-user: Ensure mmap_min_addr is non-zero
Posted by Richard Henderson 3 years, 8 months ago
When the chroot does not have /proc mounted, we can read neither
/proc/sys/vm/mmap_min_addr nor /proc/sys/maps.

The enforcement of mmap_min_addr in the host kernel is done by
the security module, and so does not apply to processes owned
by root.  Which leads pgd_find_hole_fallback to succeed in probing
a reservation at address 0.  Which confuses pgb_reserved_va to
believe that guest_base has not actually been initialized.

We don't actually want NULL addresses to become accessible, so
make sure that mmap_min_addr is initialized with a non-zero value.

Buglink: https://bugs.launchpad.net/qemu/+bug/1888728
Reported-by: John Paul Adrian Glaubitz <glaubitz@physik.fu-berlin.de>
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 linux-user/main.c | 16 ++++++++++++++--
 1 file changed, 14 insertions(+), 2 deletions(-)

diff --git a/linux-user/main.c b/linux-user/main.c
index 3597e99bb1..75c9785157 100644
--- a/linux-user/main.c
+++ b/linux-user/main.c
@@ -758,14 +758,26 @@ int main(int argc, char **argv, char **envp)
 
         if ((fp = fopen("/proc/sys/vm/mmap_min_addr", "r")) != NULL) {
             unsigned long tmp;
-            if (fscanf(fp, "%lu", &tmp) == 1) {
+            if (fscanf(fp, "%lu", &tmp) == 1 && tmp != 0) {
                 mmap_min_addr = tmp;
-                qemu_log_mask(CPU_LOG_PAGE, "host mmap_min_addr=0x%lx\n", mmap_min_addr);
+                qemu_log_mask(CPU_LOG_PAGE, "host mmap_min_addr=0x%lx\n",
+                              mmap_min_addr);
             }
             fclose(fp);
         }
     }
 
+    /*
+     * We prefer to not make NULL pointers accessible to QEMU.
+     * If we're in a chroot with no /proc, fall back to 1 page.
+     */
+    if (mmap_min_addr == 0) {
+        mmap_min_addr = qemu_host_page_size;
+        qemu_log_mask(CPU_LOG_PAGE,
+                      "host mmap_min_addr=0x%lx (fallback)\n",
+                      mmap_min_addr);
+    }
+
     /*
      * Prepare copy of argv vector for target.
      */
-- 
2.25.1


Re: [PATCH] linux-user: Ensure mmap_min_addr is non-zero
Posted by John Paul Adrian Glaubitz 3 years, 8 months ago
Hi!

On 7/24/20 11:23 PM, Richard Henderson wrote:
> When the chroot does not have /proc mounted, we can read neither
> /proc/sys/vm/mmap_min_addr nor /proc/sys/maps.
> 
> The enforcement of mmap_min_addr in the host kernel is done by
> the security module, and so does not apply to processes owned
> by root.  Which leads pgd_find_hole_fallback to succeed in probing
> a reservation at address 0.  Which confuses pgb_reserved_va to
> believe that guest_base has not actually been initialized.
> 
> We don't actually want NULL addresses to become accessible, so
> make sure that mmap_min_addr is initialized with a non-zero value.
> 
> Buglink: https://bugs.launchpad.net/qemu/+bug/1888728
> Reported-by: John Paul Adrian Glaubitz <glaubitz@physik.fu-berlin.de>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>  linux-user/main.c | 16 ++++++++++++++--
>  1 file changed, 14 insertions(+), 2 deletions(-)
> 
> diff --git a/linux-user/main.c b/linux-user/main.c
> index 3597e99bb1..75c9785157 100644
> --- a/linux-user/main.c
> +++ b/linux-user/main.c
> @@ -758,14 +758,26 @@ int main(int argc, char **argv, char **envp)
>  
>          if ((fp = fopen("/proc/sys/vm/mmap_min_addr", "r")) != NULL) {
>              unsigned long tmp;
> -            if (fscanf(fp, "%lu", &tmp) == 1) {
> +            if (fscanf(fp, "%lu", &tmp) == 1 && tmp != 0) {
>                  mmap_min_addr = tmp;
> -                qemu_log_mask(CPU_LOG_PAGE, "host mmap_min_addr=0x%lx\n", mmap_min_addr);
> +                qemu_log_mask(CPU_LOG_PAGE, "host mmap_min_addr=0x%lx\n",
> +                              mmap_min_addr);
>              }
>              fclose(fp);
>          }
>      }
>  
> +    /*
> +     * We prefer to not make NULL pointers accessible to QEMU.
> +     * If we're in a chroot with no /proc, fall back to 1 page.
> +     */
> +    if (mmap_min_addr == 0) {
> +        mmap_min_addr = qemu_host_page_size;
> +        qemu_log_mask(CPU_LOG_PAGE,
> +                      "host mmap_min_addr=0x%lx (fallback)\n",
> +                      mmap_min_addr);
> +    }
> +
>      /*
>       * Prepare copy of argv vector for target.
>       */
> 

This fixes the problem for me, therefore:

Tested-by: John Paul Adrian Glaubitz <glaubitz@physik.fu-berlin.de>

Adrian

-- 
 .''`.  John Paul Adrian Glaubitz
: :' :  Debian Developer - glaubitz@debian.org
`. `'   Freie Universitaet Berlin - glaubitz@physik.fu-berlin.de
  `-    GPG: 62FF 8A75 84E0 2956 9546  0006 7426 3B37 F5B5 F913

Re: [PATCH] linux-user: Ensure mmap_min_addr is non-zero
Posted by Laurent Vivier 3 years, 8 months ago
Le 24/07/2020 à 23:23, Richard Henderson a écrit :
> When the chroot does not have /proc mounted, we can read neither
> /proc/sys/vm/mmap_min_addr nor /proc/sys/maps.
> 
> The enforcement of mmap_min_addr in the host kernel is done by
> the security module, and so does not apply to processes owned
> by root.  Which leads pgd_find_hole_fallback to succeed in probing
> a reservation at address 0.  Which confuses pgb_reserved_va to
> believe that guest_base has not actually been initialized.
> 
> We don't actually want NULL addresses to become accessible, so
> make sure that mmap_min_addr is initialized with a non-zero value.
> 
> Buglink: https://bugs.launchpad.net/qemu/+bug/1888728
> Reported-by: John Paul Adrian Glaubitz <glaubitz@physik.fu-berlin.de>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>  linux-user/main.c | 16 ++++++++++++++--
>  1 file changed, 14 insertions(+), 2 deletions(-)
> 

Richard, do you want to add this fix in one of your branches PR?

In this case:
Acked-by: Laurent Vivier <laurent@vivier.eu>

Otherwise I can prepare a small one for linux-user, adding one or two
fixes...

Thanks,
Laurent

Re: [PATCH] linux-user: Ensure mmap_min_addr is non-zero
Posted by Richard Henderson 3 years, 8 months ago
On 7/27/20 4:19 AM, Laurent Vivier wrote:
> Le 24/07/2020 à 23:23, Richard Henderson a écrit :
>> When the chroot does not have /proc mounted, we can read neither
>> /proc/sys/vm/mmap_min_addr nor /proc/sys/maps.
>>
>> The enforcement of mmap_min_addr in the host kernel is done by
>> the security module, and so does not apply to processes owned
>> by root.  Which leads pgd_find_hole_fallback to succeed in probing
>> a reservation at address 0.  Which confuses pgb_reserved_va to
>> believe that guest_base has not actually been initialized.
>>
>> We don't actually want NULL addresses to become accessible, so
>> make sure that mmap_min_addr is initialized with a non-zero value.
>>
>> Buglink: https://bugs.launchpad.net/qemu/+bug/1888728
>> Reported-by: John Paul Adrian Glaubitz <glaubitz@physik.fu-berlin.de>
>> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
>> ---
>>  linux-user/main.c | 16 ++++++++++++++--
>>  1 file changed, 14 insertions(+), 2 deletions(-)
>>
> 
> Richard, do you want to add this fix in one of your branches PR?

I don't have anything queued at the moment.  I thought I would be able to
convince Alex to add it to his misc queue, but he sent that this morning.

> In this case:
> Acked-by: Laurent Vivier <laurent@vivier.eu>
> 
> Otherwise I can prepare a small one for linux-user, adding one or two
> fixes...

Please go ahead.


r~

Re: [PATCH] linux-user: Ensure mmap_min_addr is non-zero
Posted by Laurent Vivier 3 years, 8 months ago
Le 24/07/2020 à 23:23, Richard Henderson a écrit :
> When the chroot does not have /proc mounted, we can read neither
> /proc/sys/vm/mmap_min_addr nor /proc/sys/maps.
> 
> The enforcement of mmap_min_addr in the host kernel is done by
> the security module, and so does not apply to processes owned
> by root.  Which leads pgd_find_hole_fallback to succeed in probing
> a reservation at address 0.  Which confuses pgb_reserved_va to
> believe that guest_base has not actually been initialized.
> 
> We don't actually want NULL addresses to become accessible, so
> make sure that mmap_min_addr is initialized with a non-zero value.
> 
> Buglink: https://bugs.launchpad.net/qemu/+bug/1888728
> Reported-by: John Paul Adrian Glaubitz <glaubitz@physik.fu-berlin.de>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>  linux-user/main.c | 16 ++++++++++++++--
>  1 file changed, 14 insertions(+), 2 deletions(-)
> 
> diff --git a/linux-user/main.c b/linux-user/main.c
> index 3597e99bb1..75c9785157 100644
> --- a/linux-user/main.c
> +++ b/linux-user/main.c
> @@ -758,14 +758,26 @@ int main(int argc, char **argv, char **envp)
>  
>          if ((fp = fopen("/proc/sys/vm/mmap_min_addr", "r")) != NULL) {
>              unsigned long tmp;
> -            if (fscanf(fp, "%lu", &tmp) == 1) {
> +            if (fscanf(fp, "%lu", &tmp) == 1 && tmp != 0) {
>                  mmap_min_addr = tmp;
> -                qemu_log_mask(CPU_LOG_PAGE, "host mmap_min_addr=0x%lx\n", mmap_min_addr);
> +                qemu_log_mask(CPU_LOG_PAGE, "host mmap_min_addr=0x%lx\n",
> +                              mmap_min_addr);
>              }
>              fclose(fp);
>          }
>      }
>  
> +    /*
> +     * We prefer to not make NULL pointers accessible to QEMU.
> +     * If we're in a chroot with no /proc, fall back to 1 page.
> +     */
> +    if (mmap_min_addr == 0) {
> +        mmap_min_addr = qemu_host_page_size;
> +        qemu_log_mask(CPU_LOG_PAGE,
> +                      "host mmap_min_addr=0x%lx (fallback)\n",
> +                      mmap_min_addr);
> +    }
> +
>      /*
>       * Prepare copy of argv vector for target.
>       */
> 

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

Thanks,
Laurent