1. Patchew
  2. QEMU
  3. [PATCH 0/6] linux-user: brk() syscall fixes and armhf static binary fix
  4. View patch

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

Helge Deller posted 6 patches 2 years, 6 months ago
Maintainers: Laurent Vivier <laurent@vivier.eu>
There is a newer version of this series
[PATCH 4/6] linux-user: Fix signed math overflow in brk() syscall
Posted by Helge Deller 2 years, 6 months ago 
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
Re: [PATCH 4/6] linux-user: Fix signed math overflow in brk() syscall
Posted by Philippe Mathieu-Daudé 2 years, 6 months ago 
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 4/6] linux-user: Fix signed math overflow in brk() syscall
Posted by Helge Deller 2 years, 6 months ago 
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

Patchew 2.1-devel-b67e4c2

© 2016 - 2026 Red Hat, Inc.