| Message ID | 20230717213545.142598-5-deller@gmx.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | linux-user: brk() syscall fixes and armhf static binary fix | expand |
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.
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
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; }