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

["Philippe Mathieu-Daudé" <philmd@linaro.org>]

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.