[PATCH 0/6] linux-user: brk() syscall fixes and armhf static binary fix

[PATCH 0/6] linux-user: brk() syscall fixes and armhf static binary fix

[Helge Deller]

Commit 86f04735ac ("linux-user: Fix brk() to release pages") introduced the
possibility for userspace applications to reduce memory footprint by calling
brk() with a lower address and free up memory.
This change introduced some failures for applications with errors like
- accesing bytes above the brk heap address on the same page,
- freeing memory below the initial brk address,
and introduced a behaviour which isn't done by the kernel (e.g. zeroing
memory above brk).

This patch set fixes those issues and have been tested with existing
programs (e.g. upx).

Additionally it includes one patch to allow running static armhf executables
(e.g. fstype) which was broken since qemu-8.0.

Helge

Helge Deller (6):
  Revert "linux-user: Make sure initial brk(0) is page-aligned"
  linux-user: Fix qemu brk() to not zero bytes on current page
  linux-user: Prohibit brk() to to shrink below initial heap address
  linux-user: Fix signed math overflow in brk() syscall
  linux-user: Fix strace output for old_mmap
  linux-user: Fix qemu-arm to run static armhf binaries

 linux-user/elfload.c |  7 +++++++
 linux-user/strace.c  | 49 ++++++++++++++++++++++++++++++++++++++++----
 linux-user/syscall.c | 25 +++++++++++++---------
 3 files changed, 67 insertions(+), 14 deletions(-)

--
2.41.0

[PATCH 1/6] Revert "linux-user: Make sure initial brk(0) is page-aligned"

[Helge Deller]

This reverts commit d28b3c90cfad1a7e211ae2bce36ecb9071086129.

It just hides the real bug, and even the Linux kernel may
return page-unaligned addresses.

Signed-off-by: Helge Deller <deller@gmx.de>
Tested-by: Markus F.X.J. Oberhumer <notifications@github.com>
---
 linux-user/syscall.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index c99ef9c01e..b9527ab00f 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -806,7 +806,7 @@ static abi_ulong brk_page;

 void target_set_brk(abi_ulong new_brk)
 {
-    target_brk = TARGET_PAGE_ALIGN(new_brk);
+    target_brk = new_brk;
     brk_page = HOST_PAGE_ALIGN(target_brk);
 }

--
2.41.0

[PATCH 2/6] linux-user: Fix qemu brk() to not zero bytes on current page

[Helge Deller]

The qemu brk() implementation is too aggressive and cleans remaining bytes
on the current page above the last brk address.
But some existing applications are buggy and read or write to bytes above
their current heap address. On a phyiscal machine this does not trigger
any runtime errors (as long as the accesses happen on the same page only)
since the Linux kernel allocates only full pages and does no zeroing on
already allocated pages.

So, fix qemu to behave the same way as the kernel does. Do not touch
already allocated pages, and - when running with different page sizes of
guest and host - zero out only those memory areas where the host have a
bigger page size than the guest.

Signed-off-by: Helge Deller <deller@gmx.de>
Tested-by: Markus F.X.J. Oberhumer <notifications@github.com>
Fixes: 86f04735ac ("linux-user: Fix brk() to release pages")
Buglink: https://github.com/upx/upx/issues/683
---
 linux-user/syscall.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index b9527ab00f..f877156ed3 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -829,10 +829,8 @@ abi_long do_brk(abi_ulong brk_val)

     /* brk_val and old target_brk might be on the same page */
     if (new_brk == TARGET_PAGE_ALIGN(target_brk)) {
-        if (brk_val > target_brk) {
-            /* empty remaining bytes in (possibly larger) host page */
-            memset(g2h_untagged(target_brk), 0, new_host_brk_page - target_brk);
-        }
+        /* empty remaining bytes in (possibly larger) host page */
+        memset(g2h_untagged(new_brk), 0, new_host_brk_page - new_brk);
         target_brk = brk_val;
         return target_brk;
     }
@@ -840,7 +838,7 @@ abi_long do_brk(abi_ulong brk_val)
     /* Release heap if necesary */
     if (new_brk < target_brk) {
         /* empty remaining bytes in (possibly larger) host page */
-        memset(g2h_untagged(brk_val), 0, new_host_brk_page - brk_val);
+        memset(g2h_untagged(new_brk), 0, new_host_brk_page - new_brk);

         /* free unused host pages and set new brk_page */
         target_munmap(new_host_brk_page, brk_page - new_host_brk_page);
@@ -873,7 +871,7 @@ abi_long do_brk(abi_ulong brk_val)
          * come from the remaining part of the previous page: it may
          * contains garbage data due to a previous heap usage (grown
          * then shrunken).  */
-        memset(g2h_untagged(target_brk), 0, brk_page - target_brk);
+        memset(g2h_untagged(brk_page), 0, HOST_PAGE_ALIGN(brk_page) - brk_page);

         target_brk = brk_val;
         brk_page = new_host_brk_page;
--
2.41.0

[PATCH 3/6] linux-user: Prohibit brk() to to shrink below initial heap address

[Helge Deller]

Since commit 86f04735ac ("linux-user: Fix brk() to release pages") it's
possible for userspace applications to reduce memory footprint by
calling brk() with a lower address and free up memory. Before that guest
heap memory never was unmapped.

But the Linux kernel prohibits to reduce brk() below the initial memory
address which is set at startup by the program, while this check was
missed in commit 86f04735ac.

This patch adds the missing check by storing the initial brk value in
initial_target_brk and verify new brk addresses against that value.

Tested with the i386 upx binary from
https://github.com/upx/upx/releases/download/v4.0.2/upx-4.0.2-i386_linux.tar.xz

Signed-off-by: Helge Deller <deller@gmx.de>
Tested-by: Markus F.X.J. Oberhumer <notifications@github.com>
Fixes: 86f04735ac ("linux-user: Fix brk() to release pages")
Buglink: https://github.com/upx/upx/issues/683
---
 linux-user/syscall.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index f877156ed3..92d146f8fb 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -801,12 +801,13 @@ static inline int host_to_target_sock_type(int host_type)
     return target_type;
 }

-static abi_ulong target_brk;
+static abi_ulong target_brk, initial_target_brk;
 static abi_ulong brk_page;

 void target_set_brk(abi_ulong new_brk)
 {
     target_brk = new_brk;
+    initial_target_brk = new_brk;
     brk_page = HOST_PAGE_ALIGN(target_brk);
 }

@@ -824,6 +825,11 @@ abi_long do_brk(abi_ulong brk_val)
         return target_brk;
     }

+    /* do not allow to shrink below initial brk value */
+    if (brk_val < initial_target_brk) {
+        brk_val = initial_target_brk;
+    }
+
     new_brk = TARGET_PAGE_ALIGN(brk_val);
     new_host_brk_page = HOST_PAGE_ALIGN(brk_val);

--
2.41.0

[PATCH 4/6] linux-user: Fix signed math overflow in brk() syscall

[Helge Deller]

Fix the math overflow when calculating the new_malloc_size.

new_host_brk_page and brk_page are unsigned integers. If userspace
reduces the heap, new_host_brk_page is lower than brk_page which results
in a huge positive number (but should actually be negative).

Fix it by adding a proper check and as such make the code more readable.

Signed-off-by: Helge Deller <deller@gmx.de>
Tested-by: Markus F.X.J. Oberhumer <notifications@github.com>
Fixes: 86f04735ac ("linux-user: Fix brk() to release pages")
Buglink: https://github.com/upx/upx/issues/683
---
 linux-user/syscall.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index 92d146f8fb..aa906bedcc 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -860,12 +860,13 @@ abi_long do_brk(abi_ulong brk_val)
      * itself); instead we treat "mapped but at wrong address" as
      * a failure and unmap again.
      */
-    new_alloc_size = new_host_brk_page - brk_page;
-    if (new_alloc_size) {
+    if (new_host_brk_page > brk_page) {
+        new_alloc_size = new_host_brk_page - brk_page;
         mapped_addr = get_errno(target_mmap(brk_page, new_alloc_size,
                                         PROT_READ|PROT_WRITE,
                                         MAP_ANON|MAP_PRIVATE, 0, 0));
     } else {
+        new_alloc_size = 0;
         mapped_addr = brk_page;
     }

--
2.41.0

[PATCH 5/6] linux-user: Fix strace output for old_mmap

[Helge Deller]

The old_mmap syscall (e.g. on i386) hands over the parameters in
a struct. Adjust the strace output to print the correct values.

Signed-off-by: Helge Deller <deller@gmx.de>
Reported-by: John Reiser <notifications@github.com>
Closes: https://gitlab.com/qemu-project/qemu/-/issues/1760
---
 linux-user/strace.c | 49 +++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 45 insertions(+), 4 deletions(-)

diff --git a/linux-user/strace.c b/linux-user/strace.c
index bbd29148d4..e0ab8046ec 100644
--- a/linux-user/strace.c
+++ b/linux-user/strace.c
@@ -3767,10 +3767,24 @@ print_utimensat(CPUArchState *cpu_env, const struct syscallname *name,

 #if defined(TARGET_NR_mmap) || defined(TARGET_NR_mmap2)
 static void
-print_mmap(CPUArchState *cpu_env, const struct syscallname *name,
+print_mmap_both(CPUArchState *cpu_env, const struct syscallname *name,
            abi_long arg0, abi_long arg1, abi_long arg2,
-           abi_long arg3, abi_long arg4, abi_long arg5)
-{
+           abi_long arg3, abi_long arg4, abi_long arg5,
+           bool is_old_mmap)
+{
+    if (is_old_mmap) {
+            abi_ulong *v;
+            abi_ulong argp = arg0;
+            if (!(v = lock_user(VERIFY_READ, argp, 6 * sizeof(abi_ulong), 1)))
+                return;
+            arg0 = tswapal(v[0]);
+            arg1 = tswapal(v[1]);
+            arg2 = tswapal(v[2]);
+            arg3 = tswapal(v[3]);
+            arg4 = tswapal(v[4]);
+            arg5 = tswapal(v[5]);
+            unlock_user(v, argp, 0);
+        }
     print_syscall_prologue(name);
     print_pointer(arg0, 0);
     print_raw_param("%d", arg1, 0);
@@ -3780,7 +3794,34 @@ print_mmap(CPUArchState *cpu_env, const struct syscallname *name,
     print_raw_param("%#x", arg5, 1);
     print_syscall_epilogue(name);
 }
-#define print_mmap2     print_mmap
+#endif
+
+#if defined(TARGET_NR_mmap)
+static void
+print_mmap(CPUArchState *cpu_env, const struct syscallname *name,
+           abi_long arg0, abi_long arg1, abi_long arg2,
+           abi_long arg3, abi_long arg4, abi_long arg5)
+{
+    return print_mmap_both(cpu_env, name, arg0, arg1, arg2, arg3,
+                           arg4, arg5,
+#if defined(TARGET_NR_mmap2)
+                            true
+#else
+                            false
+#endif
+                            );
+}
+#endif
+
+#if defined(TARGET_NR_mmap2)
+static void
+print_mmap2(CPUArchState *cpu_env, const struct syscallname *name,
+           abi_long arg0, abi_long arg1, abi_long arg2,
+           abi_long arg3, abi_long arg4, abi_long arg5)
+{
+    return print_mmap_both(cpu_env, name, arg0, arg1, arg2, arg3,
+                           arg4, arg5, false);
+}
 #endif

 #ifdef TARGET_NR_mprotect
--
2.41.0

[PATCH 6/6] linux-user: Fix qemu-arm to run static armhf binaries

[Helge Deller]

qemu-user crashes immediately when running static binaries on the armhf
architecture. The problem is the memory layout where the executable is
loaded before the interpreter library, in which case the reserved brk
region clashes with the interpreter code and is released before qemu
tries to start the program.

Fix it by ncreasing the brk value to the highest brk value of
interpreter or executable.

Signed-off-by: Helge Deller <deller@gmx.de>
Reported-by:  Venkata.Pyla@toshiba-tsip.com
Closes: https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=1040981
---
 linux-user/elfload.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/linux-user/elfload.c b/linux-user/elfload.c
index a26200d9f3..94951630b1 100644
--- a/linux-user/elfload.c
+++ b/linux-user/elfload.c
@@ -3615,6 +3615,13 @@ int load_elf_binary(struct linux_binprm *bprm, struct image_info *info)

     if (elf_interpreter) {
         load_elf_interp(elf_interpreter, &interp_info, bprm->buf);
+        /*
+         * adjust brk address if the interpreter was loaded above the main
+         * executable, e.g. happens with static binaries on armhf
+         */
+        if (interp_info.brk > info->brk) {
+            info->brk = interp_info.brk;
+        }

         /* If the program interpreter is one of these two, then assume
            an iBCS2 image.  Otherwise assume a native linux image.  */
--
2.41.0

Re: [PATCH 0/6] linux-user: brk() syscall fixes and armhf static binary fix

[Philippe Mathieu-Daudé]

On 17/7/23 23:35, Helge Deller wrote:
> Commit 86f04735ac ("linux-user: Fix brk() to release pages") introduced the
> possibility for userspace applications to reduce memory footprint by calling
> brk() with a lower address and free up memory.
> This change introduced some failures for applications with errors like
> - accesing bytes above the brk heap address on the same page,
> - freeing memory below the initial brk address,
> and introduced a behaviour which isn't done by the kernel (e.g. zeroing
> memory above brk).
> 
> This patch set fixes those issues and have been tested with existing
> programs (e.g. upx).
> 
> Additionally it includes one patch to allow running static armhf executables
> (e.g. fstype) which was broken since qemu-8.0.
> 
> Helge
> 
> Helge Deller (6):
>    Revert "linux-user: Make sure initial brk(0) is page-aligned"
>    linux-user: Fix qemu brk() to not zero bytes on current page
>    linux-user: Prohibit brk() to to shrink below initial heap address
>    linux-user: Fix signed math overflow in brk() syscall
>    linux-user: Fix strace output for old_mmap
>    linux-user: Fix qemu-arm to run static armhf binaries

I'm not sure this series will reach Markus at
Markus F.X.J. Oberhumer <notifications@github.com> =)
Cc'ing his "minilzo.h" address.

Re: [PATCH 4/6] linux-user: Fix signed math overflow in brk() syscall

[Philippe Mathieu-Daudé]

On 17/7/23 23:35, Helge Deller wrote:
> Fix the math overflow when calculating the new_malloc_size.
> 
> new_host_brk_page and brk_page are unsigned integers. If userspace
> reduces the heap, new_host_brk_page is lower than brk_page which results
> in a huge positive number (but should actually be negative).
> 
> Fix it by adding a proper check and as such make the code more readable.
> 
> Signed-off-by: Helge Deller <deller@gmx.de>
> Tested-by: Markus F.X.J. Oberhumer <notifications@github.com>

Tested-by: Markus F.X.J. Oberhumer <markus@oberhumer.com>

> Fixes: 86f04735ac ("linux-user: Fix brk() to release pages")

Hmm isn't it:

Fixes: ef4330c23b ("linux-user: Handle brk() attempts with very large 
sizes")

?

> Buglink: https://github.com/upx/upx/issues/683

Also:

Cc: qemu-stable@nongnu.org

> ---
>   linux-user/syscall.c | 5 +++--
>   1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/linux-user/syscall.c b/linux-user/syscall.c
> index 92d146f8fb..aa906bedcc 100644
> --- a/linux-user/syscall.c
> +++ b/linux-user/syscall.c
> @@ -860,12 +860,13 @@ abi_long do_brk(abi_ulong brk_val)
>        * itself); instead we treat "mapped but at wrong address" as
>        * a failure and unmap again.
>        */
> -    new_alloc_size = new_host_brk_page - brk_page;
> -    if (new_alloc_size) {
> +    if (new_host_brk_page > brk_page) {
> +        new_alloc_size = new_host_brk_page - brk_page;
>           mapped_addr = get_errno(target_mmap(brk_page, new_alloc_size,
>                                           PROT_READ|PROT_WRITE,
>                                           MAP_ANON|MAP_PRIVATE, 0, 0));
>       } else {
> +        new_alloc_size = 0;
>           mapped_addr = brk_page;
>       }
> 
> --
> 2.41.0

Alternatively:

-- >8 --
diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index 1464151826..aafb13f3b4 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -814,7 +814,7 @@ void target_set_brk(abi_ulong new_brk)
  abi_long do_brk(abi_ulong brk_val)
  {
      abi_long mapped_addr;
-    abi_ulong new_alloc_size;
+    abi_long new_alloc_size;
      abi_ulong new_brk, new_host_brk_page;

      /* brk pointers are always untagged */
@@ -857,8 +857,8 @@ abi_long do_brk(abi_ulong brk_val)
       * a failure and unmap again.
       */
      new_alloc_size = new_host_brk_page - brk_page;
-    if (new_alloc_size) {
-        mapped_addr = get_errno(target_mmap(brk_page, new_alloc_size,
+    if (new_alloc_size > 0) {
+        mapped_addr = get_errno(target_mmap(brk_page, 
(abi_ulong)new_alloc_size,
                                          PROT_READ|PROT_WRITE,
                                          MAP_ANON|MAP_PRIVATE, 0, 0));
      } else {
---

Anyhow,

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Thanks!

Phil.

Re: [PATCH 0/6] linux-user: brk() syscall fixes and armhf static binary fix

[Song Gao]

Hi,  Helge

Could you see the following bugs:
https://gitlab.com/qemu-project/qemu/-/issues/1707

This issue is also caused by  the commit  86f04735ac.

Thanks.
Song Gao


在 2023/7/18 上午5:35, Helge Deller 写道:
> Commit 86f04735ac ("linux-user: Fix brk() to release pages") introduced the
> possibility for userspace applications to reduce memory footprint by calling
> brk() with a lower address and free up memory.
> This change introduced some failures for applications with errors like
> - accesing bytes above the brk heap address on the same page,
> - freeing memory below the initial brk address,
> and introduced a behaviour which isn't done by the kernel (e.g. zeroing
> memory above brk).
>
> This patch set fixes those issues and have been tested with existing
> programs (e.g. upx).
>
> Additionally it includes one patch to allow running static armhf executables
> (e.g. fstype) which was broken since qemu-8.0.
>
> Helge
>
> Helge Deller (6):
>    Revert "linux-user: Make sure initial brk(0) is page-aligned"
>    linux-user: Fix qemu brk() to not zero bytes on current page
>    linux-user: Prohibit brk() to to shrink below initial heap address
>    linux-user: Fix signed math overflow in brk() syscall
>    linux-user: Fix strace output for old_mmap
>    linux-user: Fix qemu-arm to run static armhf binaries
>
>   linux-user/elfload.c |  7 +++++++
>   linux-user/strace.c  | 49 ++++++++++++++++++++++++++++++++++++++++----
>   linux-user/syscall.c | 25 +++++++++++++---------
>   3 files changed, 67 insertions(+), 14 deletions(-)
>
> --
> 2.41.0
>

Re: [PATCH 6/6] linux-user: Fix qemu-arm to run static armhf binaries

[Michael Tokarev]

18.07.2023 00:35, Helge Deller wrote:
> qemu-user crashes immediately when running static binaries on the armhf
> architecture. The problem is the memory layout where the executable is
> loaded before the interpreter library, in which case the reserved brk
> region clashes with the interpreter code and is released before qemu
> tries to start the program.
> 
> Fix it by ncreasing the brk value to the highest brk value of
> interpreter or executable.

Nitpick: increasing, not ncreasing.

/mjt

Re: [PATCH 0/6] linux-user: brk() syscall fixes and armhf static binary fix

[Helge Deller]

On 7/18/23 05:03, Song Gao wrote:
> Hi,  Helge
>
> Could you see the following bugs:
> https://gitlab.com/qemu-project/qemu/-/issues/1707
>
> This issue is also caused by  the commit  86f04735ac.

I don't have access to such a box (and on an arm64 debian porterbox
I get unmet build dependencies, e.g. for gcc-powerpc64-linux-gnu).

If you can provide me with access to a machine I can test,
otherwise you may simply check out:
   git pull https://github.com/hdeller/qemu-hppa.git linux-user-brk-fixes
and test yourself.

Helge


>
> Thanks.
> Song Gao
>
>
> 在 2023/7/18 上午5:35, Helge Deller 写道:
>> Commit 86f04735ac ("linux-user: Fix brk() to release pages") introduced the
>> possibility for userspace applications to reduce memory footprint by calling
>> brk() with a lower address and free up memory.
>> This change introduced some failures for applications with errors like
>> - accesing bytes above the brk heap address on the same page,
>> - freeing memory below the initial brk address,
>> and introduced a behaviour which isn't done by the kernel (e.g. zeroing
>> memory above brk).
>>
>> This patch set fixes those issues and have been tested with existing
>> programs (e.g. upx).
>>
>> Additionally it includes one patch to allow running static armhf executables
>> (e.g. fstype) which was broken since qemu-8.0.
>>
>> Helge
>>
>> Helge Deller (6):
>>    Revert "linux-user: Make sure initial brk(0) is page-aligned"
>>    linux-user: Fix qemu brk() to not zero bytes on current page
>>    linux-user: Prohibit brk() to to shrink below initial heap address
>>    linux-user: Fix signed math overflow in brk() syscall
>>    linux-user: Fix strace output for old_mmap
>>    linux-user: Fix qemu-arm to run static armhf binaries
>>
>>   linux-user/elfload.c |  7 +++++++
>>   linux-user/strace.c  | 49 ++++++++++++++++++++++++++++++++++++++++----
>>   linux-user/syscall.c | 25 +++++++++++++---------
>>   3 files changed, 67 insertions(+), 14 deletions(-)
>>
>> --
>> 2.41.0
>>
>

Re: [PATCH 0/6] linux-user: brk() syscall fixes and armhf static binary fix

[Song Gao]

在 2023/7/18 下午1:42, Helge Deller 写道:
> On 7/18/23 05:03, Song Gao wrote:
>> Hi,  Helge
>>
>> Could you see the following bugs:
>> https://gitlab.com/qemu-project/qemu/-/issues/1707
>>
>> This issue is also caused by  the commit  86f04735ac.
>
> I don't have access to such a box (and on an arm64 debian porterbox
> I get unmet build dependencies, e.g. for gcc-powerpc64-linux-gnu).
>
> If you can provide me with access to a machine I can test,
> otherwise you may simply check out:
>   git pull https://github.com/hdeller/qemu-hppa.git linux-user-brk-fixes
> and test yourself.
>
Thank you.
I update code to the lastet,   it's no problem.

Thanks.
Song Gao
> Helge
>>
>> Thanks.
>> Song Gao
>>
>>
>> 在 2023/7/18 上午5:35, Helge Deller 写道:
>>> Commit 86f04735ac ("linux-user: Fix brk() to release pages") 
>>> introduced the
>>> possibility for userspace applications to reduce memory footprint by 
>>> calling
>>> brk() with a lower address and free up memory.
>>> This change introduced some failures for applications with errors like
>>> - accesing bytes above the brk heap address on the same page,
>>> - freeing memory below the initial brk address,
>>> and introduced a behaviour which isn't done by the kernel (e.g. zeroing
>>> memory above brk).
>>>
>>> This patch set fixes those issues and have been tested with existing
>>> programs (e.g. upx).
>>>
>>> Additionally it includes one patch to allow running static armhf 
>>> executables
>>> (e.g. fstype) which was broken since qemu-8.0.
>>>
>>> Helge
>>>
>>> Helge Deller (6):
>>>    Revert "linux-user: Make sure initial brk(0) is page-aligned"
>>>    linux-user: Fix qemu brk() to not zero bytes on current page
>>>    linux-user: Prohibit brk() to to shrink below initial heap address
>>>    linux-user: Fix signed math overflow in brk() syscall
>>>    linux-user: Fix strace output for old_mmap
>>>    linux-user: Fix qemu-arm to run static armhf binaries
>>>
>>>   linux-user/elfload.c |  7 +++++++
>>>   linux-user/strace.c  | 49 
>>> ++++++++++++++++++++++++++++++++++++++++----
>>>   linux-user/syscall.c | 25 +++++++++++++---------
>>>   3 files changed, 67 insertions(+), 14 deletions(-)
>>>
>>> -- 
>>> 2.41.0
>>>
>>

Re: [PATCH 0/6] linux-user: brk() syscall fixes and armhf static binary fix

[Michael Tokarev]

18.07.2023 06:03, Song Gao пишет:
> Hi,  Helge
> 
> Could you see the following bugs:
> https://gitlab.com/qemu-project/qemu/-/issues/1707
> 
> This issue is also caused by  the commit  86f04735ac.

This issue has been fixed in master already and even in 8.0.3 stable release
(I haven't checked which commit did that, though).

/mjt

Re: [PATCH 1/6] Revert "linux-user: Make sure initial brk(0) is page-aligned"

[Andreas Schwab]

On Jul 17 2023, Helge Deller wrote:

> This reverts commit d28b3c90cfad1a7e211ae2bce36ecb9071086129.
>
> It just hides the real bug, and even the Linux kernel may
> return page-unaligned addresses.

The initial brk is always page aligned, see binfmt_elf.c:set_brk and the
various arch_randomize_brk implementations.

-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."

Re: [PATCH 1/6] Revert "linux-user: Make sure initial brk(0) is page-aligned"

[Helge Deller]

On 7/18/23 15:53, Andreas Schwab wrote:
> On Jul 17 2023, Helge Deller wrote:
>
>> This reverts commit d28b3c90cfad1a7e211ae2bce36ecb9071086129.
>>
>> It just hides the real bug, and even the Linux kernel may
>> return page-unaligned addresses.
>
> The initial brk is always page aligned, see binfmt_elf.c:set_brk and the
> various arch_randomize_brk implementations.

Oh, your are absolutely right. I indeed missed to look at binfmt_elf.c:set_brk().
I'll drop this patch in the v2 series.

Thanks!
Helge

Re: [PATCH 4/6] linux-user: Fix signed math overflow in brk() syscall

[Helge Deller]

On 7/18/23 00:02, Philippe Mathieu-Daudé wrote:
> On 17/7/23 23:35, Helge Deller wrote:
>> Fix the math overflow when calculating the new_malloc_size.
>>
>> new_host_brk_page and brk_page are unsigned integers. If userspace
>> reduces the heap, new_host_brk_page is lower than brk_page which results
>> in a huge positive number (but should actually be negative).
>>
>> Fix it by adding a proper check and as such make the code more readable.
>>
>> Signed-off-by: Helge Deller <deller@gmx.de>
>> Tested-by: Markus F.X.J. Oberhumer <notifications@github.com>
>
> Tested-by: Markus F.X.J. Oberhumer <markus@oberhumer.com>

Ok.


>> Fixes: 86f04735ac ("linux-user: Fix brk() to release pages")
>
> Hmm isn't it:
>
> Fixes: ef4330c23b ("linux-user: Handle brk() attempts with very large sizes")

It's really 86f04735ac because this one introduced freeing of memory which
can lead to new_host_brk_page becoming smaller than  brk_page.

>> Buglink: https://github.com/upx/upx/issues/683
>
> Also:
>
> Cc: qemu-stable@nongnu.org

Yep.

>
>> ---
>>   linux-user/syscall.c | 5 +++--
>>   1 file changed, 3 insertions(+), 2 deletions(-)
>>
>> diff --git a/linux-user/syscall.c b/linux-user/syscall.c
>> index 92d146f8fb..aa906bedcc 100644
>> --- a/linux-user/syscall.c
>> +++ b/linux-user/syscall.c
>> @@ -860,12 +860,13 @@ abi_long do_brk(abi_ulong brk_val)
>>        * itself); instead we treat "mapped but at wrong address" as
>>        * a failure and unmap again.
>>        */
>> -    new_alloc_size = new_host_brk_page - brk_page;
>> -    if (new_alloc_size) {
>> +    if (new_host_brk_page > brk_page) {
>> +        new_alloc_size = new_host_brk_page - brk_page;
>>           mapped_addr = get_errno(target_mmap(brk_page, new_alloc_size,
>>                                           PROT_READ|PROT_WRITE,
>>                                           MAP_ANON|MAP_PRIVATE, 0, 0));
>>       } else {
>> +        new_alloc_size = 0;
>>           mapped_addr = brk_page;
>>       }
>>
>> --
>> 2.41.0
>
> Alternatively:
>
> -- >8 --
> diff --git a/linux-user/syscall.c b/linux-user/syscall.c
> index 1464151826..aafb13f3b4 100644
> --- a/linux-user/syscall.c
> +++ b/linux-user/syscall.c
> @@ -814,7 +814,7 @@ void target_set_brk(abi_ulong new_brk)
>   abi_long do_brk(abi_ulong brk_val)
>   {
>       abi_long mapped_addr;
> -    abi_ulong new_alloc_size;
> +    abi_long new_alloc_size;
>       abi_ulong new_brk, new_host_brk_page;
>
>       /* brk pointers are always untagged */
> @@ -857,8 +857,8 @@ abi_long do_brk(abi_ulong brk_val)
>        * a failure and unmap again.
>        */
>       new_alloc_size = new_host_brk_page - brk_page;
> -    if (new_alloc_size) {
> -        mapped_addr = get_errno(target_mmap(brk_page, new_alloc_size,
> +    if (new_alloc_size > 0) {
> +        mapped_addr = get_errno(target_mmap(brk_page, (abi_ulong)new_alloc_size,
>                                           PROT_READ|PROT_WRITE,
>                                           MAP_ANON|MAP_PRIVATE, 0, 0));
>       } else {

possible, but I like my patch more.

> Anyhow,
>
> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Thanks!

Helge

Re: [PATCH 0/6] linux-user: brk() syscall fixes and armhf static binary fix

[Michael Tokarev]

18.07.2023 11:30, Michael Tokarev wrote:
> 18.07.2023 06:03, Song Gao пишет:
>> Hi,  Helge
>>
>> Could you see the following bugs:
>> https://gitlab.com/qemu-project/qemu/-/issues/1707
>>
>> This issue is also caused by  the commit  86f04735ac.
> 
> This issue has been fixed in master already and even in 8.0.3 stable release
> (I haven't checked which commit did that, though).

This claim turned out to be false: the prob is fixed in *debian* build of
qemu v8.0.3, which includes an additional change on top of qemu v8.0.3,
"linux-user: Make sure initial brk(0) is page-aligned" - the one which is being
reverted in this patchset, apparenly incorrectly.

So, in short, https://gitlab.com/qemu-project/qemu/-/issues/1707 is fixed
in qemu master but not in qemu v8.0.3 stable. Hopefully the fix will be in v8.0.4
(together with other fixes from this thread).

Thanks,  and sorry for my mistake.

/mjt