Re: [PATCH] x86/dumpstack: Dump user space code correctly again

* Re: [PATCH] x86/dumpstack: Dump user space code correctly again
       [not found] ` <87h7tz306w.fsf@nanos.tec.linutronix.de>
@ 2020-07-22 18:04   ` H.J. Lu
  2020-07-22 19:47     ` Thomas Gleixner
  2020-07-22 21:55   ` [tip: x86/urgent] " tip-bot2 for Thomas Gleixner
  2020-07-23  8:19   ` [PATCH] " Christoph Hellwig
  2 siblings, 1 reply; 6+ messages in thread
From: H.J. Lu @ 2020-07-22 18:04 UTC (permalink / raw)
  To: Thomas Gleixner
  Cc: LKML, the arch/x86 maintainers, Christoph Hellwig, Josh Poimboeuf

On Wed, Jul 22, 2020 at 10:54 AM Thomas Gleixner <tglx@linutronix.de> wrote:
>
> Subject: x86/dumpstack: Dump user space code correctly again
> From: Thomas Gleixner <tglx@linutronix.de>
> Date: Wed, 22 Jul 2020 10:39:54 +0200
>
> H.J. reported that post 5.7 a segfault of a user space task does not longer
> dump the Code bytes when /proc/sys/debug/exception-trace is enabled. It
> prints 'Code: Bad RIP value.' instead.
>
> This was broken by a recent change which made probe_kernel_read() reject
> non-kernel addresses.
>
> Update show_opcodes() so it retrieves user space opcodes via
> copy_from_user_nmi().
>
> Fixes: 98a23609b103 ("maccess: always use strict semantics for probe_kernel_read")
> Reported-by: H.J. Lu <hjl.tools@gmail.com>
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> ---
>  arch/x86/kernel/dumpstack.c |   27 +++++++++++++++++----------
>  1 file changed, 17 insertions(+), 10 deletions(-)
>
> --- a/arch/x86/kernel/dumpstack.c
> +++ b/arch/x86/kernel/dumpstack.c
> @@ -71,6 +71,22 @@ static void printk_stack_address(unsigne
>         printk("%s %s%pB\n", log_lvl, reliable ? "" : "? ", (void *)address);
>  }
>
> +static int copy_code(struct pt_regs *regs, u8 *buf, unsigned long src,
> +                    unsigned int nbytes)
> +{
> +       if (!user_mode(regs))
> +               return copy_from_kernel_nofault(buf, (u8 *)src, nbytes);
> +
> +       /*
> +        * Make sure userspace isn't trying to trick us into dumping kernel
> +        * memory by pointing the userspace instruction pointer at it.
> +        */
> +       if (__chk_range_not_ok(src, nbytes, TASK_SIZE_MAX))
> +               return -EINVAL;
> +
> +       return copy_from_user_nmi(buf, (void __user *)src, nbytes);
> +}
> +
>  /*
>   * There are a couple of reasons for the 2/3rd prologue, courtesy of Linus:
>   *
> @@ -97,17 +113,8 @@ void show_opcodes(struct pt_regs *regs,
>  #define OPCODE_BUFSIZE (PROLOGUE_SIZE + 1 + EPILOGUE_SIZE)
>         u8 opcodes[OPCODE_BUFSIZE];
>         unsigned long prologue = regs->ip - PROLOGUE_SIZE;
> -       bool bad_ip;
> -
> -       /*
> -        * Make sure userspace isn't trying to trick us into dumping kernel
> -        * memory by pointing the userspace instruction pointer at it.
> -        */
> -       bad_ip = user_mode(regs) &&
> -               __chk_range_not_ok(prologue, OPCODE_BUFSIZE, TASK_SIZE_MAX);
>
> -       if (bad_ip || copy_from_kernel_nofault(opcodes, (u8 *)prologue,
> -                                       OPCODE_BUFSIZE)) {
> +       if (copy_code(regs, opcodes, prologue, sizeof(opcodes))) {
>                 printk("%sCode: Bad RIP value.\n", loglvl);
>         } else {
>                 printk("%sCode: %" __stringify(PROLOGUE_SIZE) "ph <%02x> %"

Add a kerel self test?

-- 
H.J.

^ permalink raw reply	[flat|nested] 6+ messages in thread