diff mbox series

opal-prd: handle devtmpfs mounted with noexec

Message ID 20201011013330.1317003-1-gyakovlev@gentoo.org
State Superseded
Headers show
Series opal-prd: handle devtmpfs mounted with noexec | expand

Checks

Context Check Description
snowpatch_ozlabs/snowpatch_job_snowpatch-skiboot-dco success Signed-off-by present
snowpatch_ozlabs/snowpatch_job_snowpatch-skiboot success Test snowpatch/job/snowpatch-skiboot on branch master
snowpatch_ozlabs/apply_patch success Successfully applied on branch master (f901fcafae14d38e29f1cc11440086ee678785d0)

Commit Message

Georgy Yakovlev Oct. 11, 2020, 1:33 a.m. UTC
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(-)

Comments

Oliver O'Halloran Oct. 11, 2020, 9:59 p.m. UTC | #1
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
Georgy Yakovlev Oct. 12, 2020, 2:56 a.m. UTC | #2
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
Oliver O'Halloran Oct. 12, 2020, 6:21 a.m. UTC | #3
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 mbox series

Patch

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;