| Message ID | 20201011013330.1317003-1-gyakovlev@gentoo.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | opal-prd: handle devtmpfs mounted with noexec | expand |
| Context | Check | Description |
|---|---|---|
| snowpatch_ozlabs/apply_patch | success | Successfully applied on branch master (f901fcafae14d38e29f1cc11440086ee678785d0) |
| snowpatch_ozlabs/snowpatch_job_snowpatch-skiboot | success | Test snowpatch/job/snowpatch-skiboot on branch master |
| snowpatch_ozlabs/snowpatch_job_snowpatch-skiboot-dco | success | Signed-off-by present |
On Sun, Oct 11, 2020 at 12:34 PM Georgy Yakovlev <gyakovlev@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@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@lists.ozlabs.org > https://lists.ozlabs.org/listinfo/skiboot
On 10/11/20 1:59 PM, Oliver O'Halloran wrote: > On Sun, Oct 11, 2020 at 12:34 PM Georgy Yakovlev <gyakovlev@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@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@lists.ozlabs.org >> https://lists.ozlabs.org/listinfo/skiboot
On Mon, Oct 12, 2020 at 1:56 PM Georgy Yakovlev <gyakovlev@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
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) { + 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); + 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;
Closes: https://github.com/open-power/skiboot/issues/258 Signed-off-by: Georgy Yakovlev <gyakovlev@gentoo.org> --- external/opal-prd/opal-prd.c | 39 ++++++++++++++++++++++++++++++++++-- 1 file changed, 37 insertions(+), 2 deletions(-)