[Skiboot] [PATCH] opal-prd: handle devtmpfs mounted with noexec

[Oliver O'Halloran]

On Sun, Oct 11, 2020 at 12:34 PM Georgy Yakovlev <gyakovlev at gentoo.org> wrote:
>
> Closes: https://github.com/open-power/skiboot/issues/258

Github / bugzilla links aren't a substitute for a commit message.

> Signed-off-by: Georgy Yakovlev <gyakovlev at gentoo.org>
> ---
>  external/opal-prd/opal-prd.c | 39 ++++++++++++++++++++++++++++++++++--
>  1 file changed, 37 insertions(+), 2 deletions(-)
>
> diff --git a/external/opal-prd/opal-prd.c b/external/opal-prd/opal-prd.c
> index d74d8039..eb9a0b38 100644
> --- a/external/opal-prd/opal-prd.c
> +++ b/external/opal-prd/opal-prd.c
> @@ -973,7 +973,10 @@ static int map_hbrt_file(struct opal_prd_ctx *ctx, const char *name)
>  static int map_hbrt_physmem(struct opal_prd_ctx *ctx, const char *name)
>  {
>         struct prd_range *range;
> +       int rc;
>         void *buf;
> +       void *ro_buf;
> +       void *rwx_buf;
>
>         range = find_range(name, 0);
>         if (!range) {
> @@ -981,15 +984,47 @@ static int map_hbrt_physmem(struct opal_prd_ctx *ctx, const char *name)
>                 return -1;
>         }
>
> -       buf = mmap(NULL, range->size, PROT_READ | PROT_WRITE | PROT_EXEC,
> +       ro_buf = mmap(NULL, range->size, PROT_READ,
>                         MAP_PRIVATE, ctx->fd, range->physaddr);
> -       if (buf == MAP_FAILED) {
> +       if (ro_buf == MAP_FAILED) {
>                 pr_log(LOG_ERR, "IMAGE: mmap(range:%s, "
>                                 "phys:0x%016lx, size:0x%016lx) failed: %m",
>                                 name, range->physaddr, range->size);
>                 return -1;
>         }
>
> +       rwx_buf = mmap(NULL, range->size, PROT_WRITE,
> +                       MAP_SHARED | MAP_ANONYMOUS, -1 , 0);
> +       if (rwx_buf == MAP_FAILED) {
> +               pr_log(LOG_ERR, "IMAGE: anon mmap(size:0x%016lx) failed: %m",
> +                               range->size);
> +               return -1;
> +       }
> +

> +       buf = memcpy(rwx_buf, ro_buf, range->size);
> +       if (!buf) {

memcpy() just returns the dest pointer so the condition will never be
hit unless rwx_buf is NULL to begin with.

> +               pr_log(LOG_ERR, "IMAGE: memcpy(dest:%p, "
> +                               "src: %p, size:0x%016lx) failed: %m",
> +                               rwx_buf, ro_buf, range->size);
> +               return -1;
> +       }
> +
> +       rc = munmap(ro_buf, range->size);
> +       if (rc < 0) {
> +               pr_log(LOG_ERR, "IMAGE: munmap("
> +                               "phys:0x%016lx, size:0x%016lx) failed: %m",
> +                               range->physaddr, range->size);
> +               return -1;
> +       }
> +
> +       rc = mprotect(buf, range->size, PROT_READ | PROT_WRITE | PROT_EXEC);

This should be PROT_READ | PROT_EXEC. You almost never want memory to
be writable and executable at the same time. There's some legitimate
uses for W+X memory (mainly JITs), but for the most part it just gives
people writing exploits a convenient place to drop shellcode.

> +       if (rc < 0) {
> +               pr_log(LOG_ERR, "IMAGE: mprotect(phys:%p, "
> +                       "size:0x%016lx, rwx) failed: %m",
> +                       buf, range->size);
> +               return -1;
> +       }
> +
>         ctx->code_addr = buf;
>         ctx->code_size = range->size;
>         return 0;
> --
> 2.28.0
>
> _______________________________________________
> Skiboot mailing list
> Skiboot at lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/skiboot