Re: [Qemu-arm] [Qemu-devel] [PATCH v6 2/6] linux-user: Validate mmap/mprotect prot value

[Aleksandar Markovic]

On Jun 5, 2019 11:13 PM, "Richard Henderson" <address@hidden> 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 <address@hidden>
> ---
>  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
>
>