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

[Oliver O'Halloran]

On Mon, Oct 12, 2020 at 1:56 PM Georgy Yakovlev <gyakovlev at gentoo.org> wrote:
>
> On 10/11/20 1:59 PM, Oliver O'Halloran wrote:
> >> +       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.
> So don't check it at all? I see it called in other places with no checking.

Yeah, I don't think I've ever seen the return value of lib memcpy()'s
ever being checked. Linux's copy_to_from_user() and friends will
return the number of bytes copied to indicate when a page fault (or
similar) occurs, but for regular memcpy there's not much point.

> >> +               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.
>
> I'm afraid not, but not sure.
> Original code maps it as rwx and afaik it tries to do something that
> requires write perms, but I can't figure it out myself.
>
> If I remove PROT_WRITE it will segfault in call_hbrt_init() with SEGV_ACCERR

Ah crap, fixing this is going to be a nightmare. Oh well, leave it as
RWX I guess.

Oliver