[PATCH] powerpc: Don't print kernel instructions in show_user_instructions()

[Jann Horn]

+cc x86 folks

On Thu, Oct 18, 2018 at 1:18 PM Christophe LEROY
<christophe.leroy at c-s.fr> wrote:
> Le 18/10/2018 à 13:12, Jann Horn a écrit :
> > On Thu, Oct 18, 2018 at 11:28 AM Christophe LEROY
> > <christophe.leroy at c-s.fr> wrote:
> >> Le 05/10/2018 à 15:21, Michael Ellerman a écrit :
> >>> Recently we implemented show_user_instructions() which dumps the code
> >>> around the NIP when a user space process dies with an unhandled
> >>> signal. This was modelled on the x86 code, and we even went so far as
> >>> to implement the exact same bug, namely that if the user process
> >>> crashed with its NIP pointing into the kernel we will dump kernel text
> >>> to dmesg. eg:
> >>>
> >>>     bad-bctr[2996]: segfault (11) at c000000000010000 nip c000000000010000 lr 12d0b0894 code 1
> >>>     bad-bctr[2996]: code: fbe10068 7cbe2b78 7c7f1b78 fb610048 38a10028 38810020 fb810050 7f8802a6
> >>>     bad-bctr[2996]: code: 3860001c f8010080 48242371 60000000 <7c7b1b79> 4082002c e8010080 eb610048
> >>>
> >>> This was discovered on x86 by Jann Horn and fixed in commit
> >>> 342db04ae712 ("x86/dumpstack: Don't dump kernel memory based on usermode RIP").
> >>>
> >>> Fix it by checking the adjusted NIP value (pc) and number of
> >>> instructions against USER_DS, and bail if we fail the check, eg:
> >>>
> >>>     bad-bctr[2969]: segfault (11) at c000000000010000 nip c000000000010000 lr 107930894 code 1
> >>>     bad-bctr[2969]: Bad NIP, not dumping instructions.
> >>>
> >>> Fixes: 88b0fe175735 ("powerpc: Add show_user_instructions()")
> >>> Signed-off-by: Michael Ellerman <mpe at ellerman.id.au>
> >>> ---
> >>>    arch/powerpc/kernel/process.c | 10 ++++++++++
> >>>    1 file changed, 10 insertions(+)
> >>>
> >>> diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.c
> >>> index 913c5725cdb2..bb6ac471a784 100644
> >>> --- a/arch/powerpc/kernel/process.c
> >>> +++ b/arch/powerpc/kernel/process.c
> >>> @@ -1306,6 +1306,16 @@ void show_user_instructions(struct pt_regs *regs)
> >>>
> >>>        pc = regs->nip - (instructions_to_print * 3 / 4 * sizeof(int));
> >>>
> >>> +     /*
> >>> +      * Make sure the NIP points at userspace, not kernel text/data or
> >>> +      * elsewhere.
> >>> +      */
> >>> +     if (!__access_ok(pc, instructions_to_print * sizeof(int), USER_DS)) {
> >>> +             pr_info("%s[%d]: Bad NIP, not dumping instructions.\n",
> >>> +                     current->comm, current->pid);
> >>> +             return;
> >>> +     }
> >>> +
> >>
> >> Is there any reason for not using access_ok() here ?
> >
> > It's probably more robust this way, in case someone decides to call
> > into this from kernel exception context at some point, or something
> > like that?
> >
>
> But access_ok() uses current->thread.addr_limit, while USER_DS may
> provide a larger segment:
>
> #ifdef __powerpc64__
> /* We use TASK_SIZE_USER64 as TASK_SIZE is not constant */
> #define USER_DS         MAKE_MM_SEG(TASK_SIZE_USER64 - 1)
> #else
> #define USER_DS         MAKE_MM_SEG(TASK_SIZE - 1)
> #endif

Where do you write a smaller value than USER_DS into the addr_limit?

The kernel is full of places that assume that any access up to USER_DS
is safe; for example, perf_output_sample_ustack(),
get_perf_callchain(), do_exit(), flush_old_exec(), vma_dump_size(),
... - and I also don't see anything in the powerpc code that would
ever write a smaller value into the addr_limit.

I don't know powerpc well, but AFAIK the rule on X86 is basically that
even for compat tasks, attempting to access anything up to USER_DS is
safe because the kernel doesn't put any kernel mappings there. Is that
different on powerpc?