Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <bug-208655-6666@https.bugzilla.kernel.org/> <87h7tz306w.fsf@nanos.tec.linutronix.de>
In-Reply-To: <87h7tz306w.fsf@nanos.tec.linutronix.de>
From:   "H.J. Lu" <hjl.tools@gmail.com>
Date:   Wed, 22 Jul 2020 11:04:44 -0700
Message-ID: <CAMe9rOrER7Mg_YvzuqguS-GCyP-bD2hogWeo+OMXm7F6JXQUDQ@mail.gmail.com>
Subject: Re: [PATCH] x86/dumpstack: Dump user space code correctly again
To:     Thomas Gleixner <tglx@linutronix.de>
Cc:     LKML <linux-kernel@vger.kernel.org>,
        "the arch/x86 maintainers" <x86@kernel.org>,
        Christoph Hellwig <hch@lst.de>,
        Josh Poimboeuf <jpoimboe@redhat.com>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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.