diff mbox series

[v6,2/6] linux-user: Validate mmap/mprotect prot value

Message ID 20190605205706.569-3-richard.henderson@linaro.org
State Superseded
Headers show
Series linux-user/aarch64: Support PROT_BTI | expand

Commit Message

Richard Henderson June 5, 2019, 8:57 p.m. UTC 
The kernel will return -EINVAL for bits set in the prot argument
that are unknown or invalid.  Previously we were simply cropping
out the bits that we care about.

Introduce validate_prot_to_pageflags to perform this check in a
single place between the two syscalls.  Differentiate between
the target and host versions of prot.  Compute the qemu internal
page_flags value at the same time.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>

---
 linux-user/mmap.c | 106 ++++++++++++++++++++++++++++++++--------------
 1 file changed, 75 insertions(+), 31 deletions(-)

-- 
2.17.1

Comments

Aleksandar Markovic June 6, 2019, 11:24 a.m. UTC | #1 
On Jun 5, 2019 11:13 PM, "Richard Henderson" <richard.henderson@linaro.org>
wrote:
>

> The kernel will return -EINVAL for bits set in the prot argument

> that are unknown or invalid.  Previously we were simply cropping

> out the bits that we care about.

>

> Introduce validate_prot_to_pageflags to perform this check in a

> single place between the two syscalls.  Differentiate between

> the target and host versions of prot.  Compute the qemu internal

> page_flags value at the same time.

>

> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>

> ---

>  linux-user/mmap.c | 106 ++++++++++++++++++++++++++++++++--------------

>  1 file changed, 75 insertions(+), 31 deletions(-)

>

> diff --git a/linux-user/mmap.c b/linux-user/mmap.c

> index af41339d57..3117f57fd8 100644

> --- a/linux-user/mmap.c

> +++ b/linux-user/mmap.c

> @@ -61,11 +61,38 @@ void mmap_fork_end(int child)

>          pthread_mutex_unlock(&mmap_mutex);

>  }

>

> +/*

> + * Validate target prot bitmask.

> + * Return the prot bitmask for the host in *HOST_PROT.

> + * Return 0 if the target prot bitmask is invalid, otherwise

> + * the internal qemu page_flags (which will include PAGE_VALID).

> + */

> +static int validate_prot_to_pageflags(int *host_prot, int prot)

> +{

> +    int valid = PROT_READ | PROT_WRITE | PROT_EXEC | TARGET_PROT_SEM;

> +    int page_flags = (prot & PAGE_BITS) | PAGE_VALID;

> +

> +    /*

> +     * For the host, we need not pass anything except read/write/exec.

> +     * While PROT_SEM is allowed by all hosts, it is also ignored, so

> +     * don't bother transforming guest bit to host bit.


I don't think that making assumptions based on an undocumented behavior is
the best practice.

Your “Let's don't bother” about such easy to implement things may create a
lot of bothering in future.

Regards,
Aleksandar

>  Any other

> +     * target-specific prot bits will not be understood by the host

> +     * and will need to be encoded into page_flags for qemu emulation.

> +     *

> +     * TODO: We do not actually have to map guest pages as executable,

> +     * since they will not be directly executed by the host.  We only

> +     * need to remember exec within page_flags.

> +     */

> +    *host_prot = prot & (PROT_READ | PROT_WRITE | PROT_EXEC);

> +

> +    return prot & ~valid ? 0 : page_flags;

> +}

> +

>  /* NOTE: all the constants are the HOST ones, but addresses are target.

*/
> -int target_mprotect(abi_ulong start, abi_ulong len, int prot)

> +int target_mprotect(abi_ulong start, abi_ulong len, int target_prot)

>  {

>      abi_ulong end, host_start, host_end, addr;

> -    int prot1, ret;

> +    int prot1, ret, page_flags, host_prot;

>

>  #ifdef DEBUG_MMAP

>      printf("mprotect: start=0x" TARGET_ABI_FMT_lx

> @@ -75,56 +102,65 @@ int target_mprotect(abi_ulong start, abi_ulong len,

int prot)
>             prot & PROT_EXEC ? 'x' : '-');

>  #endif

>

> -    if ((start & ~TARGET_PAGE_MASK) != 0)

> +    if ((start & ~TARGET_PAGE_MASK) != 0) {

>          return -TARGET_EINVAL;

> +    }

> +    page_flags = validate_prot_to_pageflags(&host_prot, target_prot);

> +    if (!page_flags) {

> +        return -TARGET_EINVAL;

> +    }

>      len = TARGET_PAGE_ALIGN(len);

>      end = start + len;

>      if (!guest_range_valid(start, len)) {

>          return -TARGET_ENOMEM;

>      }

> -    prot &= PROT_READ | PROT_WRITE | PROT_EXEC;

> -    if (len == 0)

> +    if (len == 0) {

>          return 0;

> +    }

>

>      mmap_lock();

>      host_start = start & qemu_host_page_mask;

>      host_end = HOST_PAGE_ALIGN(end);

>      if (start > host_start) {

>          /* handle host page containing start */

> -        prot1 = prot;

> -        for(addr = host_start; addr < start; addr += TARGET_PAGE_SIZE) {

> +        prot1 = host_prot;

> +        for (addr = host_start; addr < start; addr += TARGET_PAGE_SIZE) {

>              prot1 |= page_get_flags(addr);

>          }

>          if (host_end == host_start + qemu_host_page_size) {

> -            for(addr = end; addr < host_end; addr += TARGET_PAGE_SIZE) {

> +            for (addr = end; addr < host_end; addr += TARGET_PAGE_SIZE) {

>                  prot1 |= page_get_flags(addr);

>              }

>              end = host_end;

>          }

> -        ret = mprotect(g2h(host_start), qemu_host_page_size, prot1 &

PAGE_BITS);
> -        if (ret != 0)

> +        ret = mprotect(g2h(host_start), qemu_host_page_size,

> +                       prot1 & PAGE_BITS);

> +        if (ret != 0) {

>              goto error;

> +        }

>          host_start += qemu_host_page_size;

>      }

>      if (end < host_end) {

> -        prot1 = prot;

> -        for(addr = end; addr < host_end; addr += TARGET_PAGE_SIZE) {

> +        prot1 = host_prot;

> +        for (addr = end; addr < host_end; addr += TARGET_PAGE_SIZE) {

>              prot1 |= page_get_flags(addr);

>          }

> -        ret = mprotect(g2h(host_end - qemu_host_page_size),

qemu_host_page_size,
> -                       prot1 & PAGE_BITS);

> -        if (ret != 0)

> +        ret = mprotect(g2h(host_end - qemu_host_page_size),

> +                       qemu_host_page_size, prot1 & PAGE_BITS);

> +        if (ret != 0) {

>              goto error;

> +        }

>          host_end -= qemu_host_page_size;

>      }

>

>      /* handle the pages in the middle */

>      if (host_start < host_end) {

> -        ret = mprotect(g2h(host_start), host_end - host_start, prot);

> -        if (ret != 0)

> +        ret = mprotect(g2h(host_start), host_end - host_start,

host_prot);
> +        if (ret != 0) {

>              goto error;

> +        }

>      }

> -    page_set_flags(start, start + len, prot | PAGE_VALID);

> +    page_set_flags(start, start + len, page_flags);

>      mmap_unlock();

>      return 0;

>  error:

> @@ -364,10 +400,11 @@ abi_ulong mmap_find_vma(abi_ulong start, abi_ulong

size, abi_ulong align)
>  }

>

>  /* NOTE: all the constants are the HOST ones */

> -abi_long target_mmap(abi_ulong start, abi_ulong len, int prot,

> +abi_long target_mmap(abi_ulong start, abi_ulong len, int target_prot,

>                       int flags, int fd, abi_ulong offset)

>  {

>      abi_ulong ret, end, real_start, real_end, retaddr, host_offset,

host_len;
> +    int page_flags, host_prot;

>

>      mmap_lock();

>  #ifdef DEBUG_MMAP

> @@ -402,6 +439,12 @@ abi_long target_mmap(abi_ulong start, abi_ulong len,

int prot,
>          goto fail;

>      }

>

> +    page_flags = validate_prot_to_pageflags(&host_prot, target_prot);

> +    if (!page_flags) {

> +        errno = EINVAL;

> +        goto fail;

> +    }

> +

>      /* Also check for overflows... */

>      len = TARGET_PAGE_ALIGN(len);

>      if (!len) {

> @@ -467,14 +510,15 @@ abi_long target_mmap(abi_ulong start, abi_ulong

len, int prot,
>          /* Note: we prefer to control the mapping address. It is

>             especially important if qemu_host_page_size >

>             qemu_real_host_page_size */

> -        p = mmap(g2h(start), host_len, prot,

> +        p = mmap(g2h(start), host_len, host_prot,

>                   flags | MAP_FIXED | MAP_ANONYMOUS, -1, 0);

> -        if (p == MAP_FAILED)

> +        if (p == MAP_FAILED) {

>              goto fail;

> +        }

>          /* update start so that it points to the file position at

'offset' */
>          host_start = (unsigned long)p;

>          if (!(flags & MAP_ANONYMOUS)) {

> -            p = mmap(g2h(start), len, prot,

> +            p = mmap(g2h(start), len, host_prot,

>                       flags | MAP_FIXED, fd, host_offset);

>              if (p == MAP_FAILED) {

>                  munmap(g2h(start), host_len);

> @@ -508,19 +552,19 @@ abi_long target_mmap(abi_ulong start, abi_ulong

len, int prot,
>              /* msync() won't work here, so we return an error if write is

>                 possible while it is a shared mapping */

>              if ((flags & MAP_TYPE) == MAP_SHARED &&

> -                (prot & PROT_WRITE)) {

> +                (host_prot & PROT_WRITE)) {

>                  errno = EINVAL;

>                  goto fail;

>              }

> -            retaddr = target_mmap(start, len, prot | PROT_WRITE,

> +            retaddr = target_mmap(start, len, target_prot | PROT_WRITE,

>                                    MAP_FIXED | MAP_PRIVATE |

MAP_ANONYMOUS,
>                                    -1, 0);

>              if (retaddr == -1)

>                  goto fail;

>              if (pread(fd, g2h(start), len, offset) == -1)

>                  goto fail;

> -            if (!(prot & PROT_WRITE)) {

> -                ret = target_mprotect(start, len, prot);

> +            if (!(host_prot & PROT_WRITE)) {

> +                ret = target_mprotect(start, len, target_prot);

>                  assert(ret == 0);

>              }

>              goto the_end;

> @@ -531,13 +575,13 @@ abi_long target_mmap(abi_ulong start, abi_ulong

len, int prot,
>              if (real_end == real_start + qemu_host_page_size) {

>                  /* one single host page */

>                  ret = mmap_frag(real_start, start, end,

> -                                prot, flags, fd, offset);

> +                                host_prot, flags, fd, offset);

>                  if (ret == -1)

>                      goto fail;

>                  goto the_end1;

>              }

>              ret = mmap_frag(real_start, start, real_start +

qemu_host_page_size,
> -                            prot, flags, fd, offset);

> +                            host_prot, flags, fd, offset);

>              if (ret == -1)

>                  goto fail;

>              real_start += qemu_host_page_size;

> @@ -546,7 +590,7 @@ abi_long target_mmap(abi_ulong start, abi_ulong len,

int prot,
>          if (end < real_end) {

>              ret = mmap_frag(real_end - qemu_host_page_size,

>                              real_end - qemu_host_page_size, end,

> -                            prot, flags, fd,

> +                            host_prot, flags, fd,

>                              offset + real_end - qemu_host_page_size -

start);
>              if (ret == -1)

>                  goto fail;

> @@ -562,13 +606,13 @@ abi_long target_mmap(abi_ulong start, abi_ulong

len, int prot,
>              else

>                  offset1 = offset + real_start - start;

>              p = mmap(g2h(real_start), real_end - real_start,

> -                     prot, flags, fd, offset1);

> +                     host_prot, flags, fd, offset1);

>              if (p == MAP_FAILED)

>                  goto fail;

>          }

>      }

>   the_end1:

> -    page_set_flags(start, start + len, prot | PAGE_VALID);

> +    page_set_flags(start, start + len, page_flags);

>   the_end:

>  #ifdef DEBUG_MMAP

>      printf("ret=0x" TARGET_ABI_FMT_lx "\n", start);

> --

> 2.17.1

>

>
diff mbox series

Patch

diff --git a/linux-user/mmap.c b/linux-user/mmap.c
index af41339d57..3117f57fd8 100644
--- a/linux-user/mmap.c
+++ b/linux-user/mmap.c
@@ -61,11 +61,38 @@  void mmap_fork_end(int child)
         pthread_mutex_unlock(&mmap_mutex);
 }
 
+/*
+ * Validate target prot bitmask.
+ * Return the prot bitmask for the host in *HOST_PROT.
+ * Return 0 if the target prot bitmask is invalid, otherwise
+ * the internal qemu page_flags (which will include PAGE_VALID).
+ */
+static int validate_prot_to_pageflags(int *host_prot, int prot)
+{
+    int valid = PROT_READ | PROT_WRITE | PROT_EXEC | TARGET_PROT_SEM;
+    int page_flags = (prot & PAGE_BITS) | PAGE_VALID;
+
+    /*
+     * For the host, we need not pass anything except read/write/exec.
+     * While PROT_SEM is allowed by all hosts, it is also ignored, so
+     * don't bother transforming guest bit to host bit.  Any other
+     * target-specific prot bits will not be understood by the host
+     * and will need to be encoded into page_flags for qemu emulation.
+     *
+     * TODO: We do not actually have to map guest pages as executable,
+     * since they will not be directly executed by the host.  We only
+     * need to remember exec within page_flags.
+     */
+    *host_prot = prot & (PROT_READ | PROT_WRITE | PROT_EXEC);
+
+    return prot & ~valid ? 0 : page_flags;
+}
+
 /* NOTE: all the constants are the HOST ones, but addresses are target. */
-int target_mprotect(abi_ulong start, abi_ulong len, int prot)
+int target_mprotect(abi_ulong start, abi_ulong len, int target_prot)
 {
     abi_ulong end, host_start, host_end, addr;
-    int prot1, ret;
+    int prot1, ret, page_flags, host_prot;
 
 #ifdef DEBUG_MMAP
     printf("mprotect: start=0x" TARGET_ABI_FMT_lx
@@ -75,56 +102,65 @@  int target_mprotect(abi_ulong start, abi_ulong len, int prot)
            prot & PROT_EXEC ? 'x' : '-');
 #endif
 
-    if ((start & ~TARGET_PAGE_MASK) != 0)
+    if ((start & ~TARGET_PAGE_MASK) != 0) {
         return -TARGET_EINVAL;
+    }
+    page_flags = validate_prot_to_pageflags(&host_prot, target_prot);
+    if (!page_flags) {
+        return -TARGET_EINVAL;
+    }
     len = TARGET_PAGE_ALIGN(len);
     end = start + len;
     if (!guest_range_valid(start, len)) {
         return -TARGET_ENOMEM;
     }
-    prot &= PROT_READ | PROT_WRITE | PROT_EXEC;
-    if (len == 0)
+    if (len == 0) {
         return 0;
+    }
 
     mmap_lock();
     host_start = start & qemu_host_page_mask;
     host_end = HOST_PAGE_ALIGN(end);
     if (start > host_start) {
         /* handle host page containing start */
-        prot1 = prot;
-        for(addr = host_start; addr < start; addr += TARGET_PAGE_SIZE) {
+        prot1 = host_prot;
+        for (addr = host_start; addr < start; addr += TARGET_PAGE_SIZE) {
             prot1 |= page_get_flags(addr);
         }
         if (host_end == host_start + qemu_host_page_size) {
-            for(addr = end; addr < host_end; addr += TARGET_PAGE_SIZE) {
+            for (addr = end; addr < host_end; addr += TARGET_PAGE_SIZE) {
                 prot1 |= page_get_flags(addr);
             }
             end = host_end;
         }
-        ret = mprotect(g2h(host_start), qemu_host_page_size, prot1 & PAGE_BITS);
-        if (ret != 0)
+        ret = mprotect(g2h(host_start), qemu_host_page_size,
+                       prot1 & PAGE_BITS);
+        if (ret != 0) {
             goto error;
+        }
         host_start += qemu_host_page_size;
     }
     if (end < host_end) {
-        prot1 = prot;
-        for(addr = end; addr < host_end; addr += TARGET_PAGE_SIZE) {
+        prot1 = host_prot;
+        for (addr = end; addr < host_end; addr += TARGET_PAGE_SIZE) {
             prot1 |= page_get_flags(addr);
         }
-        ret = mprotect(g2h(host_end - qemu_host_page_size), qemu_host_page_size,
-                       prot1 & PAGE_BITS);
-        if (ret != 0)
+        ret = mprotect(g2h(host_end - qemu_host_page_size),
+                       qemu_host_page_size, prot1 & PAGE_BITS);
+        if (ret != 0) {
             goto error;
+        }
         host_end -= qemu_host_page_size;
     }
 
     /* handle the pages in the middle */
     if (host_start < host_end) {
-        ret = mprotect(g2h(host_start), host_end - host_start, prot);
-        if (ret != 0)
+        ret = mprotect(g2h(host_start), host_end - host_start, host_prot);
+        if (ret != 0) {
             goto error;
+        }
     }
-    page_set_flags(start, start + len, prot | PAGE_VALID);
+    page_set_flags(start, start + len, page_flags);
     mmap_unlock();
     return 0;
 error:
@@ -364,10 +400,11 @@  abi_ulong mmap_find_vma(abi_ulong start, abi_ulong size, abi_ulong align)
 }
 
 /* NOTE: all the constants are the HOST ones */
-abi_long target_mmap(abi_ulong start, abi_ulong len, int prot,
+abi_long target_mmap(abi_ulong start, abi_ulong len, int target_prot,
                      int flags, int fd, abi_ulong offset)
 {
     abi_ulong ret, end, real_start, real_end, retaddr, host_offset, host_len;
+    int page_flags, host_prot;
 
     mmap_lock();
 #ifdef DEBUG_MMAP
@@ -402,6 +439,12 @@  abi_long target_mmap(abi_ulong start, abi_ulong len, int prot,
         goto fail;
     }
 
+    page_flags = validate_prot_to_pageflags(&host_prot, target_prot);
+    if (!page_flags) {
+        errno = EINVAL;
+        goto fail;
+    }
+
     /* Also check for overflows... */
     len = TARGET_PAGE_ALIGN(len);
     if (!len) {
@@ -467,14 +510,15 @@  abi_long target_mmap(abi_ulong start, abi_ulong len, int prot,
         /* Note: we prefer to control the mapping address. It is
            especially important if qemu_host_page_size >
            qemu_real_host_page_size */
-        p = mmap(g2h(start), host_len, prot,
+        p = mmap(g2h(start), host_len, host_prot,
                  flags | MAP_FIXED | MAP_ANONYMOUS, -1, 0);
-        if (p == MAP_FAILED)
+        if (p == MAP_FAILED) {
             goto fail;
+        }
         /* update start so that it points to the file position at 'offset' */
         host_start = (unsigned long)p;
         if (!(flags & MAP_ANONYMOUS)) {
-            p = mmap(g2h(start), len, prot,
+            p = mmap(g2h(start), len, host_prot,
                      flags | MAP_FIXED, fd, host_offset);
             if (p == MAP_FAILED) {
                 munmap(g2h(start), host_len);
@@ -508,19 +552,19 @@  abi_long target_mmap(abi_ulong start, abi_ulong len, int prot,
             /* msync() won't work here, so we return an error if write is
                possible while it is a shared mapping */
             if ((flags & MAP_TYPE) == MAP_SHARED &&
-                (prot & PROT_WRITE)) {
+                (host_prot & PROT_WRITE)) {
                 errno = EINVAL;
                 goto fail;
             }
-            retaddr = target_mmap(start, len, prot | PROT_WRITE,
+            retaddr = target_mmap(start, len, target_prot | PROT_WRITE,
                                   MAP_FIXED | MAP_PRIVATE | MAP_ANONYMOUS,
                                   -1, 0);
             if (retaddr == -1)
                 goto fail;
             if (pread(fd, g2h(start), len, offset) == -1)
                 goto fail;
-            if (!(prot & PROT_WRITE)) {
-                ret = target_mprotect(start, len, prot);
+            if (!(host_prot & PROT_WRITE)) {
+                ret = target_mprotect(start, len, target_prot);
                 assert(ret == 0);
             }
             goto the_end;
@@ -531,13 +575,13 @@  abi_long target_mmap(abi_ulong start, abi_ulong len, int prot,
             if (real_end == real_start + qemu_host_page_size) {
                 /* one single host page */
                 ret = mmap_frag(real_start, start, end,
-                                prot, flags, fd, offset);
+                                host_prot, flags, fd, offset);
                 if (ret == -1)
                     goto fail;
                 goto the_end1;
             }
             ret = mmap_frag(real_start, start, real_start + qemu_host_page_size,
-                            prot, flags, fd, offset);
+                            host_prot, flags, fd, offset);
             if (ret == -1)
                 goto fail;
             real_start += qemu_host_page_size;
@@ -546,7 +590,7 @@  abi_long target_mmap(abi_ulong start, abi_ulong len, int prot,
         if (end < real_end) {
             ret = mmap_frag(real_end - qemu_host_page_size,
                             real_end - qemu_host_page_size, end,
-                            prot, flags, fd,
+                            host_prot, flags, fd,
                             offset + real_end - qemu_host_page_size - start);
             if (ret == -1)
                 goto fail;
@@ -562,13 +606,13 @@  abi_long target_mmap(abi_ulong start, abi_ulong len, int prot,
             else
                 offset1 = offset + real_start - start;
             p = mmap(g2h(real_start), real_end - real_start,
-                     prot, flags, fd, offset1);
+                     host_prot, flags, fd, offset1);
             if (p == MAP_FAILED)
                 goto fail;
         }
     }
  the_end1:
-    page_set_flags(start, start + len, prot | PAGE_VALID);
+    page_set_flags(start, start + len, page_flags);
  the_end:
 #ifdef DEBUG_MMAP
     printf("ret=0x" TARGET_ABI_FMT_lx "\n", start);