On Wed, Jul 22, 2020 at 10:54 AM Thomas Gleixner <[email protected]> wrote:
>
> Subject: x86/dumpstack: Dump user space code correctly again
> From: Thomas Gleixner <[email protected]>
> 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 <[email protected]>
> Signed-off-by: Thomas Gleixner <[email protected]>
> ---
> 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.
"H.J. Lu" <[email protected]> writes:
> On Wed, Jul 22, 2020 at 10:54 AM Thomas Gleixner <[email protected]> wrote:
>> + 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?
Care to whip one up?
On Wed, Jul 22, 2020 at 12:47 PM Thomas Gleixner <[email protected]> wrote:
>
> "H.J. Lu" <[email protected]> writes:
> > On Wed, Jul 22, 2020 at 10:54 AM Thomas Gleixner <[email protected]> wrote:
> >> + 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?
>
> Care to whip one up?
>
Can we count "dmesg" to always dump opcode?
--
H.J.