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

[Georgy Yakovlev]

On 10/11/20 1:59 PM, Oliver O'Halloran wrote:
> 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.
Thanks. Will update after/if we figure out the rest. More below.
> 
>> 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.
So don't check it at all? I see it called in other places with no checking.

> 
>> +               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

(gdb) bt
#0  0x00007ffff748016c in ?? ()
#1  0x000000010000abc8 in call_be ()
#2  0x0000000100005448 in hservices_init (ctx=0x7fffffffd250, 
code=0x7ffff7480000) at opal-prd.c:897
#3  0x0000000100009380 in run_prd_daemon (ctx=0x7fffffffd250) at 
opal-prd.c:2287
#4  0x000000010000a914 in main (argc=3, argv=0x7fffffffd728) at 
opal-prd.c:2746


write(1, "IMAGE: calling ibm,hbrt_init()\n", 31IMAGE: calling 
ibm,hbrt_init()
) = 31
switch_endian()                         = 4963763352
--- SIGSEGV {si_signo=SIGSEGV, si_code=SEGV_ACCERR, 
si_addr=0x7fff95ca2008} ---
+++ killed by SIGSEGV (core dumped) +++
Segmentation fault

this leads to thunk.S and I'm helpless there.


> 
>> +       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