* 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* Re: [PATCH] x86/dumpstack: Dump user space code correctly again
2020-07-22 18:04 ` [PATCH] x86/dumpstack: Dump user space code correctly again H.J. Lu
@ 2020-07-22 19:47 ` Thomas Gleixner
2020-07-22 21:22 ` H.J. Lu
0 siblings, 1 reply; 6+ messages in thread
From: Thomas Gleixner @ 2020-07-22 19:47 UTC (permalink / raw)
To: H.J. Lu; +Cc: LKML, the arch/x86 maintainers, Christoph Hellwig, Josh Poimboeuf
"H.J. Lu" <hjl.tools@gmail.com> writes:
> On Wed, Jul 22, 2020 at 10:54 AM Thomas Gleixner <tglx@linutronix.de> 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?
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] x86/dumpstack: Dump user space code correctly again
2020-07-22 19:47 ` Thomas Gleixner
@ 2020-07-22 21:22 ` H.J. Lu
0 siblings, 0 replies; 6+ messages in thread
From: H.J. Lu @ 2020-07-22 21:22 UTC (permalink / raw)
To: Thomas Gleixner
Cc: LKML, the arch/x86 maintainers, Christoph Hellwig, Josh Poimboeuf
On Wed, Jul 22, 2020 at 12:47 PM Thomas Gleixner <tglx@linutronix.de> wrote:
>
> "H.J. Lu" <hjl.tools@gmail.com> writes:
> > On Wed, Jul 22, 2020 at 10:54 AM Thomas Gleixner <tglx@linutronix.de> 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.
^ permalink raw reply [flat|nested] 6+ messages in thread
* [tip: x86/urgent] x86/dumpstack: Dump user space code correctly again
[not found] ` <87h7tz306w.fsf@nanos.tec.linutronix.de>
2020-07-22 18:04 ` [PATCH] x86/dumpstack: Dump user space code correctly again H.J. Lu
@ 2020-07-22 21:55 ` tip-bot2 for Thomas Gleixner
2020-07-23 8:19 ` [PATCH] " Christoph Hellwig
2 siblings, 0 replies; 6+ messages in thread
From: tip-bot2 for Thomas Gleixner @ 2020-07-22 21:55 UTC (permalink / raw)
To: linux-tip-commits; +Cc: H.J. Lu, Thomas Gleixner, x86, LKML
The following commit has been merged into the x86/urgent branch of tip:
Commit-ID: d181d2da0141371bbc360eaea78719203e165e1c
Gitweb: https://git.kernel.org/tip/d181d2da0141371bbc360eaea78719203e165e1c
Author: Thomas Gleixner <tglx@linutronix.de>
AuthorDate: Wed, 22 Jul 2020 10:39:54 +02:00
Committer: Thomas Gleixner <tglx@linutronix.de>
CommitterDate: Wed, 22 Jul 2020 23:47:48 +02:00
x86/dumpstack: Dump user space code correctly again
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>
Link: https://lkml.kernel.org/r/87h7tz306w.fsf@nanos.tec.linutronix.de
---
arch/x86/kernel/dumpstack.c | 27 +++++++++++++++++----------
1 file changed, 17 insertions(+), 10 deletions(-)
diff --git a/arch/x86/kernel/dumpstack.c b/arch/x86/kernel/dumpstack.c
index b037cfa..7401cc1 100644
--- a/arch/x86/kernel/dumpstack.c
+++ b/arch/x86/kernel/dumpstack.c
@@ -71,6 +71,22 @@ static void printk_stack_address(unsigned long address, int reliable,
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, const char *loglvl)
#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> %"
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH] x86/dumpstack: Dump user space code correctly again
[not found] ` <87h7tz306w.fsf@nanos.tec.linutronix.de>
2020-07-22 18:04 ` [PATCH] x86/dumpstack: Dump user space code correctly again H.J. Lu
2020-07-22 21:55 ` [tip: x86/urgent] " tip-bot2 for Thomas Gleixner
@ 2020-07-23 8:19 ` Christoph Hellwig
2020-07-23 11:05 ` Thomas Gleixner
2 siblings, 1 reply; 6+ messages in thread
From: Christoph Hellwig @ 2020-07-23 8:19 UTC (permalink / raw)
To: Thomas Gleixner; +Cc: LKML, H.J. Lu, x86, Christoph Hellwig, Josh Poimboeuf
On Wed, Jul 22, 2020 at 07:54:15PM +0200, Thomas Gleixner 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>
Looks good, and also cleans up the code nicely:
Reviewed-by: Christoph Hellwig <hch@lst.de>
But one question below:
> + /*
> + * 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);
copy_from_user_nmi already contains a:
if (__range_not_ok(from, n, TASK_SIZE))
return n;
what is the reason it checks for TASK_SIZE vs TASK_SIZE_MAX, and why
do we need both checks?
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] x86/dumpstack: Dump user space code correctly again
2020-07-23 8:19 ` [PATCH] " Christoph Hellwig
@ 2020-07-23 11:05 ` Thomas Gleixner
0 siblings, 0 replies; 6+ messages in thread
From: Thomas Gleixner @ 2020-07-23 11:05 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: LKML, H.J. Lu, x86, Christoph Hellwig, Josh Poimboeuf
Christoph Hellwig <hch@lst.de> writes:
> On Wed, Jul 22, 2020 at 07:54:15PM +0200, Thomas Gleixner wrote:
>> + /*
>> + * 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);
>
> copy_from_user_nmi already contains a:
>
> if (__range_not_ok(from, n, TASK_SIZE))
> return n;
>
> what is the reason it checks for TASK_SIZE vs TASK_SIZE_MAX, and why
> do we need both checks?
TBH, I just kept it because being lazy. But you are right, the check is
redundant and TASK_SIZE_MAX is inaccurate as it does not take compat
tasks into account. I'll rip that out.
Thanks,
tglx
^ permalink raw reply [flat|nested] 6+ messages in thread