diff mbox series

[09/24] linux-user: Implement MAP_FIXED_NOREPLACE

Message ID 20230630132159.376995-10-richard.henderson@linaro.org
State Superseded
Headers show
Series linux-user: mmap range fixes | expand

Commit Message

Richard Henderson June 30, 2023, 1:21 p.m. UTC
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 linux-user/mmap.c | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

Comments

Alex Bennée July 3, 2023, 9:51 a.m. UTC | #1
Richard Henderson <richard.henderson@linaro.org> writes:

> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>  linux-user/mmap.c | 14 +++++++++++---
>  1 file changed, 11 insertions(+), 3 deletions(-)
>
> diff --git a/linux-user/mmap.c b/linux-user/mmap.c
> index cb5369d2d1..41c2f09fd5 100644
> --- a/linux-user/mmap.c
> +++ b/linux-user/mmap.c
> @@ -502,7 +502,7 @@ abi_long target_mmap(abi_ulong start, abi_ulong len, int target_prot,
>       * If the user is asking for the kernel to find a location, do that
>       * before we truncate the length for mapping files below.
>       */
> -    if (!(flags & MAP_FIXED)) {
> +    if (!(flags & (MAP_FIXED | MAP_FIXED_NOREPLACE))) {
>          host_len = len + offset - host_offset;
>          host_len = HOST_PAGE_ALIGN(host_len);
>          start = mmap_find_vma(real_start, host_len, TARGET_PAGE_SIZE);
> @@ -544,7 +544,7 @@ abi_long target_mmap(abi_ulong start, abi_ulong len, int target_prot,
>          }
>      }
>  
> -    if (!(flags & MAP_FIXED)) {
> +    if (!(flags & (MAP_FIXED | MAP_FIXED_NOREPLACE))) {
>          unsigned long host_start;
>          void *p;
>  
> @@ -593,6 +593,13 @@ abi_long target_mmap(abi_ulong start, abi_ulong len, int target_prot,
>              goto fail;
>          }
>  
> +        /* Validate that the chosen range is empty. */
> +        if ((flags & MAP_FIXED_NOREPLACE)
> +            && !page_check_range_empty(start, end - 1)) {
> +            errno = EEXIST;
> +            goto fail;
> +        }
> +
>          /*
>           * worst case: we cannot map the file because the offset is not
>           * aligned, so we read it
> @@ -608,7 +615,8 @@ abi_long target_mmap(abi_ulong start, abi_ulong len, int target_prot,
>                  goto fail;
>              }
>              retaddr = target_mmap(start, len, target_prot | PROT_WRITE,
> -                                  MAP_FIXED | MAP_PRIVATE | MAP_ANONYMOUS,
> +                                  (flags & (MAP_FIXED | MAP_FIXED_NOREPLACE))
> +                                  | MAP_PRIVATE | MAP_ANONYMOUS,

I thought we always MAP_FIXED because we've already jumped the hoops to
work out where in the memory space this allocation is going. Now if the
guest doesn't specify MAP_FIXED the kernel might decide to put the
memory somewhere else, potentially out of reach of the guest addressing?

IOW I thought the MAP_FIXED here was an internal QEMU implementation
details rather than reflecting the guests wishes.

>                                    -1, 0);
>              if (retaddr == -1) {
>                  goto fail;
Richard Henderson July 3, 2023, 10:39 a.m. UTC | #2
On 7/3/23 11:51, Alex Bennée wrote:
>> @@ -544,7 +544,7 @@ abi_long target_mmap(abi_ulong start, abi_ulong len, int target_prot,
>>           }
>>       }
>>   
>> -    if (!(flags & MAP_FIXED)) {
>> +    if (!(flags & (MAP_FIXED | MAP_FIXED_NOREPLACE))) {
>>           unsigned long host_start;
>>           void *p;
>>   
...
>> @@ -608,7 +615,8 @@ abi_long target_mmap(abi_ulong start, abi_ulong len, int target_prot,
>>                   goto fail;
>>               }
>>               retaddr = target_mmap(start, len, target_prot | PROT_WRITE,
>> -                                  MAP_FIXED | MAP_PRIVATE | MAP_ANONYMOUS,
>> +                                  (flags & (MAP_FIXED | MAP_FIXED_NOREPLACE))
>> +                                  | MAP_PRIVATE | MAP_ANONYMOUS,
> 
> I thought we always MAP_FIXED because we've already jumped the hoops to
> work out where in the memory space this allocation is going.

We have not, no.  Not here, anyway.

> IOW I thought the MAP_FIXED here was an internal QEMU implementation
> details rather than reflecting the guests wishes.

As I read it, MAP_FIXED was here because we'd already checked for it in the IF that I 
retained above.  But now we're checking for 2 things, and must pass that on to the recursion.


r~
diff mbox series

Patch

diff --git a/linux-user/mmap.c b/linux-user/mmap.c
index cb5369d2d1..41c2f09fd5 100644
--- a/linux-user/mmap.c
+++ b/linux-user/mmap.c
@@ -502,7 +502,7 @@  abi_long target_mmap(abi_ulong start, abi_ulong len, int target_prot,
      * If the user is asking for the kernel to find a location, do that
      * before we truncate the length for mapping files below.
      */
-    if (!(flags & MAP_FIXED)) {
+    if (!(flags & (MAP_FIXED | MAP_FIXED_NOREPLACE))) {
         host_len = len + offset - host_offset;
         host_len = HOST_PAGE_ALIGN(host_len);
         start = mmap_find_vma(real_start, host_len, TARGET_PAGE_SIZE);
@@ -544,7 +544,7 @@  abi_long target_mmap(abi_ulong start, abi_ulong len, int target_prot,
         }
     }
 
-    if (!(flags & MAP_FIXED)) {
+    if (!(flags & (MAP_FIXED | MAP_FIXED_NOREPLACE))) {
         unsigned long host_start;
         void *p;
 
@@ -593,6 +593,13 @@  abi_long target_mmap(abi_ulong start, abi_ulong len, int target_prot,
             goto fail;
         }
 
+        /* Validate that the chosen range is empty. */
+        if ((flags & MAP_FIXED_NOREPLACE)
+            && !page_check_range_empty(start, end - 1)) {
+            errno = EEXIST;
+            goto fail;
+        }
+
         /*
          * worst case: we cannot map the file because the offset is not
          * aligned, so we read it
@@ -608,7 +615,8 @@  abi_long target_mmap(abi_ulong start, abi_ulong len, int target_prot,
                 goto fail;
             }
             retaddr = target_mmap(start, len, target_prot | PROT_WRITE,
-                                  MAP_FIXED | MAP_PRIVATE | MAP_ANONYMOUS,
+                                  (flags & (MAP_FIXED | MAP_FIXED_NOREPLACE))
+                                  | MAP_PRIVATE | MAP_ANONYMOUS,
                                   -1, 0);
             if (retaddr == -1) {
                 goto fail;