1. Patchew
  2. QEMU
  3. View series

[PATCH] linux-user/mmap.c: fix integer underflow in target_mremap

Jonathan Marler posted 1 patch 4 years ago
Test docker-mingw@fedora passed
Test checkpatch passed
Test asan passed
Test docker-quick@centos7 passed
Test FreeBSD passed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20200502161225.14346-1-johnnymarler@gmail.com
Maintainers: Laurent Vivier <laurent@vivier.eu>, Riku Voipio <riku.voipio@iki.fi>
linux-user/mmap.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
[PATCH] linux-user/mmap.c: fix integer underflow in target_mremap
Posted by Jonathan Marler 4 years ago 
Fixes: https://bugs.launchpad.net/bugs/1876373

This code path in mmap occurs when a page size is decreased with mremap.  When a section of pages is shrunk, qemu calls mmap_reserve on the pages that were released.  However, it has the diff operation reversed, subtracting the larger old_size from the smaller new_size.  Instead, it should be subtracting the smaller new_size from the larger old_size.  You can also see in the previous line of the change that this mmap_reserve call only occurs when old_size > new_size.

Signed-off-by: Jonathan Marler <johnnymarler@gmail.com>
---
 linux-user/mmap.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/linux-user/mmap.c b/linux-user/mmap.c
index e378033797..caab62909e 100644
--- a/linux-user/mmap.c
+++ b/linux-user/mmap.c
@@ -708,7 +708,7 @@ abi_long target_mremap(abi_ulong old_addr, abi_ulong old_size,
         if (prot == 0) {
             host_addr = mremap(g2h(old_addr), old_size, new_size, flags);
             if (host_addr != MAP_FAILED && reserved_va && old_size > new_size) {
-                mmap_reserve(old_addr + old_size, new_size - old_size);
+                mmap_reserve(old_addr + old_size, old_size - new_size);
             }
         } else {
             errno = ENOMEM;
-- 
2.23.1
Re: [PATCH] linux-user/mmap.c: fix integer underflow in target_mremap
Posted by Jonathan Marler 4 years ago 
FYI, I applied this patch to the qemu build that zig uses to run non-native
tests (
https://github.com/ziglang/qemu-static/blob/master/patch/mremap-underflow.diff
)

After applying it, my new code that calls mremap now passes, whereas before
the fix I was getting a segfault.

On Sat, May 2, 2020 at 10:12 AM Jonathan Marler <johnnymarler@gmail.com>
wrote:

> Fixes: https://bugs.launchpad.net/bugs/1876373
>
> This code path in mmap occurs when a page size is decreased with mremap.
> When a section of pages is shrunk, qemu calls mmap_reserve on the pages
> that were released.  However, it has the diff operation reversed,
> subtracting the larger old_size from the smaller new_size.  Instead, it
> should be subtracting the smaller new_size from the larger old_size.  You
> can also see in the previous line of the change that this mmap_reserve call
> only occurs when old_size > new_size.
>
> Signed-off-by: Jonathan Marler <johnnymarler@gmail.com>
> ---
>  linux-user/mmap.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/linux-user/mmap.c b/linux-user/mmap.c
> index e378033797..caab62909e 100644
> --- a/linux-user/mmap.c
> +++ b/linux-user/mmap.c
> @@ -708,7 +708,7 @@ abi_long target_mremap(abi_ulong old_addr, abi_ulong
> old_size,
>          if (prot == 0) {
>              host_addr = mremap(g2h(old_addr), old_size, new_size, flags);
>              if (host_addr != MAP_FAILED && reserved_va && old_size >
> new_size) {
> -                mmap_reserve(old_addr + old_size, new_size - old_size);
> +                mmap_reserve(old_addr + old_size, old_size - new_size);
>              }
>          } else {
>              errno = ENOMEM;
> --
> 2.23.1
>
>
Re: [PATCH] linux-user/mmap.c: fix integer underflow in target_mremap
Posted by Jonathan Marler 3 years, 11 months ago 
Been a couple weeks, checking to see if anyone has looked at this.

On Sat, May 2, 2020 at 5:43 PM Jonathan Marler <johnnymarler@gmail.com>
wrote:

> FYI, I applied this patch to the qemu build that zig uses to run
> non-native tests (
> https://github.com/ziglang/qemu-static/blob/master/patch/mremap-underflow.diff
> )
>
> After applying it, my new code that calls mremap now passes,
> whereas before the fix I was getting a segfault.
>
> On Sat, May 2, 2020 at 10:12 AM Jonathan Marler <johnnymarler@gmail.com>
> wrote:
>
>> Fixes: https://bugs.launchpad.net/bugs/1876373
>>
>> This code path in mmap occurs when a page size is decreased with mremap.
>> When a section of pages is shrunk, qemu calls mmap_reserve on the pages
>> that were released.  However, it has the diff operation reversed,
>> subtracting the larger old_size from the smaller new_size.  Instead, it
>> should be subtracting the smaller new_size from the larger old_size.  You
>> can also see in the previous line of the change that this mmap_reserve call
>> only occurs when old_size > new_size.
>>
>> Signed-off-by: Jonathan Marler <johnnymarler@gmail.com>
>> ---
>>  linux-user/mmap.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/linux-user/mmap.c b/linux-user/mmap.c
>> index e378033797..caab62909e 100644
>> --- a/linux-user/mmap.c
>> +++ b/linux-user/mmap.c
>> @@ -708,7 +708,7 @@ abi_long target_mremap(abi_ulong old_addr, abi_ulong
>> old_size,
>>          if (prot == 0) {
>>              host_addr = mremap(g2h(old_addr), old_size, new_size, flags);
>>              if (host_addr != MAP_FAILED && reserved_va && old_size >
>> new_size) {
>> -                mmap_reserve(old_addr + old_size, new_size - old_size);
>> +                mmap_reserve(old_addr + old_size, old_size - new_size);
>>              }
>>          } else {
>>              errno = ENOMEM;
>> --
>> 2.23.1
>>
>>
Re: [PATCH] linux-user/mmap.c: fix integer underflow in target_mremap
Posted by Jonathan Marler 3 years, 11 months ago 
Been a few more days.  Not sure how often I should be pinging.  If this is
too much to ping every few days let me know.

On Fri, May 15, 2020 at 7:36 AM Jonathan Marler <johnnymarler@gmail.com>
wrote:

> Been a couple weeks, checking to see if anyone has looked at this.
>
> On Sat, May 2, 2020 at 5:43 PM Jonathan Marler <johnnymarler@gmail.com>
> wrote:
>
>> FYI, I applied this patch to the qemu build that zig uses to run
>> non-native tests (
>> https://github.com/ziglang/qemu-static/blob/master/patch/mremap-underflow.diff
>> )
>>
>> After applying it, my new code that calls mremap now passes,
>> whereas before the fix I was getting a segfault.
>>
>> On Sat, May 2, 2020 at 10:12 AM Jonathan Marler <johnnymarler@gmail.com>
>> wrote:
>>
>>> Fixes: https://bugs.launchpad.net/bugs/1876373
>>>
>>> This code path in mmap occurs when a page size is decreased with
>>> mremap.  When a section of pages is shrunk, qemu calls mmap_reserve on the
>>> pages that were released.  However, it has the diff operation reversed,
>>> subtracting the larger old_size from the smaller new_size.  Instead, it
>>> should be subtracting the smaller new_size from the larger old_size.  You
>>> can also see in the previous line of the change that this mmap_reserve call
>>> only occurs when old_size > new_size.
>>>
>>> Signed-off-by: Jonathan Marler <johnnymarler@gmail.com>
>>> ---
>>>  linux-user/mmap.c | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/linux-user/mmap.c b/linux-user/mmap.c
>>> index e378033797..caab62909e 100644
>>> --- a/linux-user/mmap.c
>>> +++ b/linux-user/mmap.c
>>> @@ -708,7 +708,7 @@ abi_long target_mremap(abi_ulong old_addr, abi_ulong
>>> old_size,
>>>          if (prot == 0) {
>>>              host_addr = mremap(g2h(old_addr), old_size, new_size,
>>> flags);
>>>              if (host_addr != MAP_FAILED && reserved_va && old_size >
>>> new_size) {
>>> -                mmap_reserve(old_addr + old_size, new_size - old_size);
>>> +                mmap_reserve(old_addr + old_size, old_size - new_size);
>>>              }
>>>          } else {
>>>              errno = ENOMEM;
>>> --
>>> 2.23.1
>>>
>>>
Re: [PATCH] linux-user/mmap.c: fix integer underflow in target_mremap
Posted by Stefano Garzarella 3 years, 11 months ago 
Hi Jonathan,
thanks for the patch!

CCing Riku and Laurent.

On Mon, May 18, 2020 at 12:13:41PM -0600, Jonathan Marler wrote:
> Been a few more days.  Not sure how often I should be pinging.  If this is
> too much to ping every few days let me know.

Is not too much, but next time is better to CC the maintainers.
You can use 'scripts/get_maintainer.pl' to get the list of maintainers
and reviewers.

Please take a look at https://wiki.qemu.org/Contribute/SubmitAPatch

> 
> On Fri, May 15, 2020 at 7:36 AM Jonathan Marler <johnnymarler@gmail.com>
> wrote:
> 
> > Been a couple weeks, checking to see if anyone has looked at this.
> >
> > On Sat, May 2, 2020 at 5:43 PM Jonathan Marler <johnnymarler@gmail.com>
> > wrote:
> >
> >> FYI, I applied this patch to the qemu build that zig uses to run
> >> non-native tests (
> >> https://github.com/ziglang/qemu-static/blob/master/patch/mremap-underflow.diff
> >> )
> >>
> >> After applying it, my new code that calls mremap now passes,
> >> whereas before the fix I was getting a segfault.
> >>
> >> On Sat, May 2, 2020 at 10:12 AM Jonathan Marler <johnnymarler@gmail.com>
> >> wrote:
> >>
> >>> Fixes: https://bugs.launchpad.net/bugs/1876373

should be "Buglink: https://bugs.launchpad.net/bugs/1876373"

> >>>
> >>> This code path in mmap occurs when a page size is decreased with
> >>> mremap.  When a section of pages is shrunk, qemu calls mmap_reserve on the
> >>> pages that were released.  However, it has the diff operation reversed,
> >>> subtracting the larger old_size from the smaller new_size.  Instead, it
> >>> should be subtracting the smaller new_size from the larger old_size.  You
> >>> can also see in the previous line of the change that this mmap_reserve call
> >>> only occurs when old_size > new_size.

Please break the lines of the commit message (max 76 charactes per line):
https://wiki.qemu.org/Contribute/SubmitAPatch#Write_a_meaningful_commit_message

Thanks,
Stefano

> >>>
> >>> Signed-off-by: Jonathan Marler <johnnymarler@gmail.com>
> >>> ---
> >>>  linux-user/mmap.c | 2 +-
> >>>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>>
> >>> diff --git a/linux-user/mmap.c b/linux-user/mmap.c
> >>> index e378033797..caab62909e 100644
> >>> --- a/linux-user/mmap.c
> >>> +++ b/linux-user/mmap.c
> >>> @@ -708,7 +708,7 @@ abi_long target_mremap(abi_ulong old_addr, abi_ulong
> >>> old_size,
> >>>          if (prot == 0) {
> >>>              host_addr = mremap(g2h(old_addr), old_size, new_size,
> >>> flags);
> >>>              if (host_addr != MAP_FAILED && reserved_va && old_size >
> >>> new_size) {
> >>> -                mmap_reserve(old_addr + old_size, new_size - old_size);
> >>> +                mmap_reserve(old_addr + old_size, old_size - new_size);
> >>>              }
> >>>          } else {
> >>>              errno = ENOMEM;
> >>> --
> >>> 2.23.1
> >>>
> >>>
Re: [PATCH] linux-user/mmap.c: fix integer underflow in target_mremap
Posted by Philippe Mathieu-Daudé 3 years, 11 months ago 
Hi Jonathan.

On 5/19/20 10:11 AM, Stefano Garzarella wrote:
> Hi Jonathan,
> thanks for the patch!
> 
> CCing Riku and Laurent.
> 
> On Mon, May 18, 2020 at 12:13:41PM -0600, Jonathan Marler wrote:
>> Been a few more days.  Not sure how often I should be pinging.  If this is
>> too much to ping every few days let me know.

Pinging every week is fine. The problem here, as noticed by Stefano, is 
you forgot to Cc the maintainers, so they surely missed your patch.

Last time Riku sent an email to qemu-devel was more than 2 years ago, 
letling Laurent second him, then went MIA:
https://www.mail-archive.com/qemu-devel@nongnu.org/msg507843.html

I'd say count 1 week starting today.

Regards,

Phil.

> 
> Is not too much, but next time is better to CC the maintainers.
> You can use 'scripts/get_maintainer.pl' to get the list of maintainers
> and reviewers.
> 
> Please take a look at https://wiki.qemu.org/Contribute/SubmitAPatch
> 
>>
>> On Fri, May 15, 2020 at 7:36 AM Jonathan Marler <johnnymarler@gmail.com>
>> wrote:
>>
>>> Been a couple weeks, checking to see if anyone has looked at this.
>>>
>>> On Sat, May 2, 2020 at 5:43 PM Jonathan Marler <johnnymarler@gmail.com>
>>> wrote:
>>>
>>>> FYI, I applied this patch to the qemu build that zig uses to run
>>>> non-native tests (
>>>> https://github.com/ziglang/qemu-static/blob/master/patch/mremap-underflow.diff
>>>> )
>>>>
>>>> After applying it, my new code that calls mremap now passes,
>>>> whereas before the fix I was getting a segfault.
>>>>
>>>> On Sat, May 2, 2020 at 10:12 AM Jonathan Marler <johnnymarler@gmail.com>
>>>> wrote:
>>>>
>>>>> Fixes: https://bugs.launchpad.net/bugs/1876373
> 
> should be "Buglink: https://bugs.launchpad.net/bugs/1876373"
> 
>>>>>
>>>>> This code path in mmap occurs when a page size is decreased with
>>>>> mremap.  When a section of pages is shrunk, qemu calls mmap_reserve on the
>>>>> pages that were released.  However, it has the diff operation reversed,
>>>>> subtracting the larger old_size from the smaller new_size.  Instead, it
>>>>> should be subtracting the smaller new_size from the larger old_size.  You
>>>>> can also see in the previous line of the change that this mmap_reserve call
>>>>> only occurs when old_size > new_size.
> 
> Please break the lines of the commit message (max 76 charactes per line):
> https://wiki.qemu.org/Contribute/SubmitAPatch#Write_a_meaningful_commit_message
> 
> Thanks,
> Stefano
> 
>>>>>
>>>>> Signed-off-by: Jonathan Marler <johnnymarler@gmail.com>
>>>>> ---
>>>>>   linux-user/mmap.c | 2 +-
>>>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/linux-user/mmap.c b/linux-user/mmap.c
>>>>> index e378033797..caab62909e 100644
>>>>> --- a/linux-user/mmap.c
>>>>> +++ b/linux-user/mmap.c
>>>>> @@ -708,7 +708,7 @@ abi_long target_mremap(abi_ulong old_addr, abi_ulong
>>>>> old_size,
>>>>>           if (prot == 0) {
>>>>>               host_addr = mremap(g2h(old_addr), old_size, new_size,
>>>>> flags);
>>>>>               if (host_addr != MAP_FAILED && reserved_va && old_size >
>>>>> new_size) {
>>>>> -                mmap_reserve(old_addr + old_size, new_size - old_size);
>>>>> +                mmap_reserve(old_addr + old_size, old_size - new_size);
>>>>>               }
>>>>>           } else {
>>>>>               errno = ENOMEM;
>>>>> --
>>>>> 2.23.1
>>>>>
>>>>>
> 
>
Re: [PATCH] linux-user/mmap.c: fix integer underflow in target_mremap
Posted by Laurent Vivier 3 years, 11 months ago 
Le 02/05/2020 à 18:12, Jonathan Marler a écrit :
> Fixes: https://bugs.launchpad.net/bugs/1876373
> 
> This code path in mmap occurs when a page size is decreased with mremap.  When a section of pages is shrunk, qemu calls mmap_reserve on the pages that were released.  However, it has the diff operation reversed, subtracting the larger old_size from the smaller new_size.  Instead, it should be subtracting the smaller new_size from the larger old_size.  You can also see in the previous line of the change that this mmap_reserve call only occurs when old_size > new_size.
> 
> Signed-off-by: Jonathan Marler <johnnymarler@gmail.com>
> ---
>  linux-user/mmap.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/linux-user/mmap.c b/linux-user/mmap.c
> index e378033797..caab62909e 100644
> --- a/linux-user/mmap.c
> +++ b/linux-user/mmap.c
> @@ -708,7 +708,7 @@ abi_long target_mremap(abi_ulong old_addr, abi_ulong old_size,
>          if (prot == 0) {
>              host_addr = mremap(g2h(old_addr), old_size, new_size, flags);
>              if (host_addr != MAP_FAILED && reserved_va && old_size > new_size) {
> -                mmap_reserve(old_addr + old_size, new_size - old_size);
> +                mmap_reserve(old_addr + old_size, old_size - new_size);
>              }
>          } else {
>              errno = ENOMEM;
> 

Applied to my linux-user branch.

Thanks,
Laurent
Re: [PATCH] linux-user/mmap.c: fix integer underflow in target_mremap
Posted by Laurent Vivier 3 years, 11 months ago 
Le 02/05/2020 à 18:12, Jonathan Marler a écrit :
> Fixes: https://bugs.launchpad.net/bugs/1876373
> 
> This code path in mmap occurs when a page size is decreased with mremap.  When a section of pages is shrunk, qemu calls mmap_reserve on the pages that were released.  However, it has the diff operation reversed, subtracting the larger old_size from the smaller new_size.  Instead, it should be subtracting the smaller new_size from the larger old_size.  You can also see in the previous line of the change that this mmap_reserve call only occurs when old_size > new_size.
> 
> Signed-off-by: Jonathan Marler <johnnymarler@gmail.com>
> ---
>  linux-user/mmap.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/linux-user/mmap.c b/linux-user/mmap.c
> index e378033797..caab62909e 100644
> --- a/linux-user/mmap.c
> +++ b/linux-user/mmap.c
> @@ -708,7 +708,7 @@ abi_long target_mremap(abi_ulong old_addr, abi_ulong old_size,
>          if (prot == 0) {
>              host_addr = mremap(g2h(old_addr), old_size, new_size, flags);
>              if (host_addr != MAP_FAILED && reserved_va && old_size > new_size) {
> -                mmap_reserve(old_addr + old_size, new_size - old_size);
> +                mmap_reserve(old_addr + old_size, old_size - new_size);
>              }
>          } else {
>              errno = ENOMEM;
> 

Reviewed-by: Laurent Vivier <laurent@vivier.eu>

Patchew 2.1-devel-0870f84

© 2016 - 2024 Red Hat, Inc.