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

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

[Jonathan Marler]

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

[Jonathan Marler]

[-- Attachment #1: Type: text/plain, Size: 1696 bytes --]

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
>
>

[-- Attachment #2: Type: text/html, Size: 2397 bytes --]

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

[Jonathan Marler]

[-- Attachment #1: Type: text/plain, Size: 1897 bytes --]

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
>>
>>

[-- Attachment #2: Type: text/html, Size: 2859 bytes --]

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

[Jonathan Marler]

[-- Attachment #1: Type: text/plain, Size: 2157 bytes --]

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
>>>
>>>

[-- Attachment #2: Type: text/html, Size: 3361 bytes --]

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

[Stefano Garzarella]

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

[Philippe Mathieu-Daudé]

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

[Laurent Vivier]

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>

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

[Laurent Vivier]

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

[Laurent Vivier]

Hi,

does this patch replace your previous one?

Please add more details in the description, as you did in the launchpad bug.

You can also add:
Fixes: https://bugs.launchpad.net/bugs/1876373

You must also send the patch to qemu-devel@nongnu.org

Thanks,
Laurent

Le 02/05/2020 à 09:49, Jonathan Marler a écrit :
> 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;
> 

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

[Jonathan Marler]

[-- Attachment #1: Type: text/plain, Size: 1528 bytes --]

Yes the first patch was incorrect.  The second patch should be the correct
one.

Thanks for the guidance.  I have created a new patch with a "Fixes: ..."
and a description of the fix, and have sent that patch to
qemu-devel@nongnu.org

On Sat, May 2, 2020 at 2:38 AM Laurent Vivier <laurent@vivier.eu> wrote:

> Hi,
>
> does this patch replace your previous one?
>
> Please add more details in the description, as you did in the launchpad
> bug.
>
> You can also add:
> Fixes: https://bugs.launchpad.net/bugs/1876373
>
> You must also send the patch to qemu-devel@nongnu.org
>
> Thanks,
> Laurent
>
> Le 02/05/2020 à 09:49, Jonathan Marler a écrit :
> > 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;
> >
>
>

[-- Attachment #2: Type: text/html, Size: 2284 bytes --]