diff mbox series

linux-user/mmap: Avoid asserts for out of range mremap calls

Message ID 70c46e7b999bafbb01d54bfafd44b420d0b782e9.camel@linuxfoundation.org (mailing list archive)
State New, archived
Headers show
Series linux-user/mmap: Avoid asserts for out of range mremap calls | expand

Commit Message

Richard Purdie Jan. 8, 2021, 5:42 p.m. UTC 
If mremap() is called without the MREMAP_MAYMOVE flag with a start address
just before the end of memory (reserved_va) where new_size would exceed 
it (and GUEST_ADDR_MAX), the assert(end - 1 <= GUEST_ADDR_MAX) in 
page_set_flags() would trigger.

Add an extra guard to the guest_range_valid() checks to prevent this and
avoid asserting binaries when reserved_va is set.

This meant a bug I was seeing locally now gives the same behaviour 
regardless of whether reserved_va is set or not.

Signed-off-by: Richard Purdie <richard.purdie@linuxfoundation.org

Comments

Richard Purdie Jan. 22, 2021, 9:37 a.m. UTC | #1 
On Fri, 2021-01-08 at 17:42 +0000, Richard Purdie wrote:
> If mremap() is called without the MREMAP_MAYMOVE flag with a start address
> just before the end of memory (reserved_va) where new_size would exceed 
> it (and GUEST_ADDR_MAX), the assert(end - 1 <= GUEST_ADDR_MAX) in 
> page_set_flags() would trigger.
> 
> Add an extra guard to the guest_range_valid() checks to prevent this and
> avoid asserting binaries when reserved_va is set.
> 
> This meant a bug I was seeing locally now gives the same behaviour 
> regardless of whether reserved_va is set or not.
> 
> Signed-off-by: Richard Purdie <richard.purdie@linuxfoundation.org
> 
> Index: qemu-5.2.0/linux-user/mmap.c
> ===================================================================
> --- qemu-5.2.0.orig/linux-user/mmap.c
> +++ qemu-5.2.0/linux-user/mmap.c
> @@ -727,7 +727,9 @@ abi_long target_mremap(abi_ulong old_add
>  
> 
>      if (!guest_range_valid(old_addr, old_size) ||
>          ((flags & MREMAP_FIXED) &&
> -         !guest_range_valid(new_addr, new_size))) {
> +         !guest_range_valid(new_addr, new_size)) ||
> +        ((flags & MREMAP_MAYMOVE) == 0 &&
> +         !guest_range_valid(old_addr, new_size))) {
>          errno = ENOMEM;
>          return -1;
>      }
> 
> 

Any comments on this? I believe its a valid issue that needs fixing and
multiple distros appear to be carrying fixes in this area related to
this.

Cheers,

Richard
Philippe Mathieu-Daudé Jan. 22, 2021, 10:28 a.m. UTC | #2 
On 1/22/21 10:37 AM, Richard Purdie wrote:
> On Fri, 2021-01-08 at 17:42 +0000, Richard Purdie wrote:
>> If mremap() is called without the MREMAP_MAYMOVE flag with a start address
>> just before the end of memory (reserved_va) where new_size would exceed 
>> it (and GUEST_ADDR_MAX), the assert(end - 1 <= GUEST_ADDR_MAX) in 
>> page_set_flags() would trigger.
>>
>> Add an extra guard to the guest_range_valid() checks to prevent this and
>> avoid asserting binaries when reserved_va is set.
>>
>> This meant a bug I was seeing locally now gives the same behaviour 
>> regardless of whether reserved_va is set or not.
>>
>> Signed-off-by: Richard Purdie <richard.purdie@linuxfoundation.org
>>
>> Index: qemu-5.2.0/linux-user/mmap.c
>> ===================================================================
>> --- qemu-5.2.0.orig/linux-user/mmap.c
>> +++ qemu-5.2.0/linux-user/mmap.c
>> @@ -727,7 +727,9 @@ abi_long target_mremap(abi_ulong old_add
>>  
>>
>>      if (!guest_range_valid(old_addr, old_size) ||
>>          ((flags & MREMAP_FIXED) &&
>> -         !guest_range_valid(new_addr, new_size))) {
>> +         !guest_range_valid(new_addr, new_size)) ||
>> +        ((flags & MREMAP_MAYMOVE) == 0 &&
>> +         !guest_range_valid(old_addr, new_size))) {
>>          errno = ENOMEM;
>>          return -1;
>>      }
>>
>>
> 
> Any comments on this? I believe its a valid issue that needs fixing and
> multiple distros appear to be carrying fixes in this area related to
> this.

You forgot to Cc the maintainer who likely missed it due to the huge
traffic:

$ ./scripts/get_maintainer.pl -f linux-user/mmap.c
Laurent Vivier <laurent@vivier.eu> (maintainer:Linux user)

Now Cc'ed.

Regards,

Phil.
Laurent Vivier Feb. 13, 2021, 4:50 p.m. UTC | #3 
Le 08/01/2021 à 18:42, Richard Purdie a écrit :
> If mremap() is called without the MREMAP_MAYMOVE flag with a start address
> just before the end of memory (reserved_va) where new_size would exceed 
> it (and GUEST_ADDR_MAX), the assert(end - 1 <= GUEST_ADDR_MAX) in 
> page_set_flags() would trigger.
> 
> Add an extra guard to the guest_range_valid() checks to prevent this and
> avoid asserting binaries when reserved_va is set.
> 
> This meant a bug I was seeing locally now gives the same behaviour 
> regardless of whether reserved_va is set or not.
> 
> Signed-off-by: Richard Purdie <richard.purdie@linuxfoundation.org
> 
> Index: qemu-5.2.0/linux-user/mmap.c
> ===================================================================
> --- qemu-5.2.0.orig/linux-user/mmap.c
> +++ qemu-5.2.0/linux-user/mmap.c
> @@ -727,7 +727,9 @@ abi_long target_mremap(abi_ulong old_add
>  
>      if (!guest_range_valid(old_addr, old_size) ||
>          ((flags & MREMAP_FIXED) &&
> -         !guest_range_valid(new_addr, new_size))) {
> +         !guest_range_valid(new_addr, new_size)) ||
> +        ((flags & MREMAP_MAYMOVE) == 0 &&
> +         !guest_range_valid(old_addr, new_size))) {
>          errno = ENOMEM;
>          return -1;
>      }
> 
> 

Reviewed-by: Laurent Vivier <laurent@vivier.eu>
Laurent Vivier Feb. 13, 2021, 5:42 p.m. UTC | #4 
Le 08/01/2021 à 18:42, Richard Purdie a écrit :
> If mremap() is called without the MREMAP_MAYMOVE flag with a start address
> just before the end of memory (reserved_va) where new_size would exceed 
> it (and GUEST_ADDR_MAX), the assert(end - 1 <= GUEST_ADDR_MAX) in 
> page_set_flags() would trigger.
> 
> Add an extra guard to the guest_range_valid() checks to prevent this and
> avoid asserting binaries when reserved_va is set.
> 
> This meant a bug I was seeing locally now gives the same behaviour 
> regardless of whether reserved_va is set or not.
> 
> Signed-off-by: Richard Purdie <richard.purdie@linuxfoundation.org
> 
> Index: qemu-5.2.0/linux-user/mmap.c
> ===================================================================
> --- qemu-5.2.0.orig/linux-user/mmap.c
> +++ qemu-5.2.0/linux-user/mmap.c
> @@ -727,7 +727,9 @@ abi_long target_mremap(abi_ulong old_add
>  
>      if (!guest_range_valid(old_addr, old_size) ||
>          ((flags & MREMAP_FIXED) &&
> -         !guest_range_valid(new_addr, new_size))) {
> +         !guest_range_valid(new_addr, new_size)) ||
> +        ((flags & MREMAP_MAYMOVE) == 0 &&
> +         !guest_range_valid(old_addr, new_size))) {
>          errno = ENOMEM;
>          return -1;
>      }
> 

Applied to my linux-user-for-6.0 branch.

Thanks,
Laurent
diff mbox series

Patch

Index: qemu-5.2.0/linux-user/mmap.c
===================================================================
--- qemu-5.2.0.orig/linux-user/mmap.c
+++ qemu-5.2.0/linux-user/mmap.c
@@ -727,7 +727,9 @@  abi_long target_mremap(abi_ulong old_add
 
     if (!guest_range_valid(old_addr, old_size) ||
         ((flags & MREMAP_FIXED) &&
-         !guest_range_valid(new_addr, new_size))) {
+         !guest_range_valid(new_addr, new_size)) ||
+        ((flags & MREMAP_MAYMOVE) == 0 &&
+         !guest_range_valid(old_addr, new_size))) {
         errno = ENOMEM;
         return -1;
     }