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

[Helge Deller <deller@gmx.de>]

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