Re: [PATCH v2] x86/dumpstack: Fix misleading instruction pointer error message
On 11/16, Thomas Gleixner wrote: > > Subject: x86/dumpstack: Don't try to access user space code of other tasks > From: Thomas Gleixner > Date: Mon, 16 Nov 2020 22:26:52 +0100 > > sysrq-t ends up invoking show_opcodes() for each task which tries to access > the user space code of other processes which is obviously bogus. > > It either manages to dump where the foreign tasks regs->ip points to in > currents mapping or triggers a pagefault and prints "Code: Bad RIP > value.". Both is just wrong. > > Add a safeguard in copy_code() and check whether the @regs pointer matches > currents pt_regs. If not, do not even try to access it. > > While at it, add commentry why using copy_from_user_nmi() is safe in > copy_code() even if the function name suggests otherwise. > > Reported-by: Mark Mossberg > Signed-off-by: Thomas Gleixner Acked-by: Oleg Nesterov
Re: [PATCH v2] x86/dumpstack: Fix misleading instruction pointer error message
On Mon, Nov 16, 2020 at 11:01:03PM +0100, Thomas Gleixner wrote: > Subject: x86/dumpstack: Don't try to access user space code of other tasks > From: Thomas Gleixner > Date: Mon, 16 Nov 2020 22:26:52 +0100 > > sysrq-t ends up invoking show_opcodes() for each task which tries to access > the user space code of other processes which is obviously bogus. > > It either manages to dump where the foreign tasks regs->ip points to in I guess you mean here "points to valid mapping of current" or so. > currents mapping or triggers a pagefault and prints "Code: Bad RIP > value.". Both is just wrong. > > Add a safeguard in copy_code() and check whether the @regs pointer matches > currents pt_regs. If not, do not even try to access it. > > While at it, add commentry why using copy_from_user_nmi() is safe in s/commentry/commentary/ > copy_code() even if the function name suggests otherwise. > > Reported-by: Mark Mossberg This is Reported-by: Oleg > Signed-off-by: Thomas Gleixner > --- > arch/x86/kernel/dumpstack.c | 23 +++ > 1 file changed, 19 insertions(+), 4 deletions(-) > > --- a/arch/x86/kernel/dumpstack.c > +++ b/arch/x86/kernel/dumpstack.c > @@ -78,6 +78,9 @@ static int copy_code(struct pt_regs *reg > if (!user_mode(regs)) > return copy_from_kernel_nofault(buf, (u8 *)src, nbytes); > > + /* The user space code from other tasks cannot be accessed. */ > + if (regs != task_pt_regs(current)) > + return -EPERM; > /* >* Make sure userspace isn't trying to trick us into dumping kernel >* memory by pointing the userspace instruction pointer at it. > @@ -85,6 +88,12 @@ static int copy_code(struct pt_regs *reg > if (__chk_range_not_ok(src, nbytes, TASK_SIZE_MAX)) > return -EINVAL; > > + /* > + * Even if named copy_from_user_nmi() this can be invoked from > + * other contexts and will not try to resolve a pagefault, which is > + * the correct thing to do here as this code can be called from any > + * context. > + */ Can we stick the first part of this comment about "this can be invoked from other contexts" over the function definition? > return copy_from_user_nmi(buf, (void __user *)src, nbytes); > } ... With this, I see Code: only once with Sysrq-T: [ 25.491878] task:bashstate:R running task stack:0 pid: 4267 ppid: 4187 flags:0x4000 ... [ 25.497740] Code: 00 f7 d8 64 89 02 48 c7 c0 ff ff ff ff eb b3 0f 1f 80 00 00 00 00 48 8d 05 f9 61 0d 00 8b 00 85 c0 75 13 b8 01 00 00 00 0f 05 <48> 3d 00 f0 ff ff 77 54 c3 0f 1f 00 41 54 49 89 d4 55 48 89 f5 53 which is the shell doing the $ echo t > /proc/sysrq-trigger So Reviewed-by: Borislav Petkov Tested-by: Borislav Petkov Thanks! -- Regards/Gruss, Boris. https://people.kernel.org/tglx/notes-about-netiquette
Re: [PATCH v2] x86/dumpstack: Fix misleading instruction pointer error message
On Mon, Nov 16, 2020 at 3:37 PM Thomas Gleixner wrote: > > On Mon, Nov 16 2020 at 15:04, Andy Lutomirski wrote: > > > On Mon, Nov 16, 2020 at 2:01 PM Thomas Gleixner wrote: > >> arch/x86/kernel/dumpstack.c | 23 +++ > >> 1 file changed, 19 insertions(+), 4 deletions(-) > >> > >> --- a/arch/x86/kernel/dumpstack.c > >> +++ b/arch/x86/kernel/dumpstack.c > >> @@ -78,6 +78,9 @@ static int copy_code(struct pt_regs *reg > >> if (!user_mode(regs)) > >> return copy_from_kernel_nofault(buf, (u8 *)src, nbytes); > >> > >> + /* The user space code from other tasks cannot be accessed. */ > >> + if (regs != task_pt_regs(current)) > >> + return -EPERM; > > > > Depending on exactly where this gets called, this may not be > > sufficient. You should also check nmi_uaccess_okay(). > > which is what copy_from_user_nmi() already does. Whoops. I thought I checked that...
Re: [PATCH v2] x86/dumpstack: Fix misleading instruction pointer error message
On Mon, Nov 16 2020 at 15:04, Andy Lutomirski wrote: > On Mon, Nov 16, 2020 at 2:01 PM Thomas Gleixner wrote: >> arch/x86/kernel/dumpstack.c | 23 +++ >> 1 file changed, 19 insertions(+), 4 deletions(-) >> >> --- a/arch/x86/kernel/dumpstack.c >> +++ b/arch/x86/kernel/dumpstack.c >> @@ -78,6 +78,9 @@ static int copy_code(struct pt_regs *reg >> if (!user_mode(regs)) >> return copy_from_kernel_nofault(buf, (u8 *)src, nbytes); >> >> + /* The user space code from other tasks cannot be accessed. */ >> + if (regs != task_pt_regs(current)) >> + return -EPERM; > > Depending on exactly where this gets called, this may not be > sufficient. You should also check nmi_uaccess_okay(). which is what copy_from_user_nmi() already does.
Re: [PATCH v2] x86/dumpstack: Fix misleading instruction pointer error message
On Mon, Nov 16, 2020 at 2:01 PM Thomas Gleixner wrote: > arch/x86/kernel/dumpstack.c | 23 +++ > 1 file changed, 19 insertions(+), 4 deletions(-) > > --- a/arch/x86/kernel/dumpstack.c > +++ b/arch/x86/kernel/dumpstack.c > @@ -78,6 +78,9 @@ static int copy_code(struct pt_regs *reg > if (!user_mode(regs)) > return copy_from_kernel_nofault(buf, (u8 *)src, nbytes); > > + /* The user space code from other tasks cannot be accessed. */ > + if (regs != task_pt_regs(current)) > + return -EPERM; Depending on exactly where this gets called, this may not be sufficient. You should also check nmi_uaccess_okay(). --Andy
Re: [PATCH v2] x86/dumpstack: Fix misleading instruction pointer error message
On Tue, Nov 03 2020 at 19:20, Borislav Petkov wrote:
> On Tue, Nov 03, 2020 at 07:11:15PM +0100, Oleg Nesterov wrote:
>> > I'm thinking copy_code() should not use copy_from_user_nmi() if former
>> > can be called in non-atomic context too.
While copy_from_user_nmi() is named that way, it can be invoked from
other contexts as well. See the comment inside.
>> I understand, but why do you think this makes sense?
>
> Because the copy_from_user_nmi()'s name tells me that it is at least
> supposed to be called in atomic context. At least this is how I
> understand it. And in atomic context regs is supposed to belong to
> current, right?
Whatever context you are in current can only read it's own user space
obviously.
AFAICT even before the change I did, show_opcodes() did not care and
just either dumped what was available at regs->ip in the current tasks
user space mapping or faulted.
Fix below.
Thanks,
tglx
---
Subject: x86/dumpstack: Don't try to access user space code of other tasks
From: Thomas Gleixner
Date: Mon, 16 Nov 2020 22:26:52 +0100
sysrq-t ends up invoking show_opcodes() for each task which tries to access
the user space code of other processes which is obviously bogus.
It either manages to dump where the foreign tasks regs->ip points to in
currents mapping or triggers a pagefault and prints "Code: Bad RIP
value.". Both is just wrong.
Add a safeguard in copy_code() and check whether the @regs pointer matches
currents pt_regs. If not, do not even try to access it.
While at it, add commentry why using copy_from_user_nmi() is safe in
copy_code() even if the function name suggests otherwise.
Reported-by: Mark Mossberg
Signed-off-by: Thomas Gleixner
---
arch/x86/kernel/dumpstack.c | 23 +++
1 file changed, 19 insertions(+), 4 deletions(-)
--- a/arch/x86/kernel/dumpstack.c
+++ b/arch/x86/kernel/dumpstack.c
@@ -78,6 +78,9 @@ static int copy_code(struct pt_regs *reg
if (!user_mode(regs))
return copy_from_kernel_nofault(buf, (u8 *)src, nbytes);
+ /* The user space code from other tasks cannot be accessed. */
+ if (regs != task_pt_regs(current))
+ return -EPERM;
/*
* Make sure userspace isn't trying to trick us into dumping kernel
* memory by pointing the userspace instruction pointer at it.
@@ -85,6 +88,12 @@ static int copy_code(struct pt_regs *reg
if (__chk_range_not_ok(src, nbytes, TASK_SIZE_MAX))
return -EINVAL;
+ /*
+* Even if named copy_from_user_nmi() this can be invoked from
+* other contexts and will not try to resolve a pagefault, which is
+* the correct thing to do here as this code can be called from any
+* context.
+*/
return copy_from_user_nmi(buf, (void __user *)src, nbytes);
}
@@ -115,13 +124,19 @@ void show_opcodes(struct pt_regs *regs,
u8 opcodes[OPCODE_BUFSIZE];
unsigned long prologue = regs->ip - PROLOGUE_SIZE;
- if (copy_code(regs, opcodes, prologue, sizeof(opcodes))) {
- printk("%sCode: Unable to access opcode bytes at RIP 0x%lx.\n",
- loglvl, prologue);
- } else {
+ switch (copy_code(regs, opcodes, prologue, sizeof(opcodes))) {
+ case 0:
printk("%sCode: %" __stringify(PROLOGUE_SIZE) "ph <%02x> %"
__stringify(EPILOGUE_SIZE) "ph\n", loglvl, opcodes,
opcodes[PROLOGUE_SIZE], opcodes + PROLOGUE_SIZE + 1);
+ break;
+ case -EPERM:
+ /* No access to the user space stack of other tasks. Ignore. */
+ break;
+ default:
+ printk("%sCode: Unable to access opcode bytes at RIP 0x%lx.\n",
+ loglvl, prologue);
+ break;
}
}
Re: [PATCH v2] x86/dumpstack: Fix misleading instruction pointer error message
On Tue, Nov 03, 2020 at 07:11:15PM +0100, Oleg Nesterov wrote: > > I'm thinking copy_code() should not use copy_from_user_nmi() if former > > can be called in non-atomic context too. > > I understand, but why do you think this makes sense? Because the copy_from_user_nmi()'s name tells me that it is at least supposed to be called in atomic context. At least this is how I understand it. And in atomic context regs is supposed to belong to current, right? So I kinda agree with what you're proposing but if copy_from_user_nmi() can be "tricked" into reading off from the weeds, then there should be a big fat warning above it at least so that users are warned to do the appropriate checks. Or there should be another wrapper around it which does the regs-belongs-to-current checks, etc and copy_code() should use that wrapper... AFAICT at least. -- Regards/Gruss, Boris. https://people.kernel.org/tglx/notes-about-netiquette
Re: [PATCH v2] x86/dumpstack: Fix misleading instruction pointer error message
On 11/03, Borislav Petkov wrote: > > On Tue, Nov 03, 2020 at 06:47:44PM +0100, Oleg Nesterov wrote: > > > I'm thinking this should not use the atomic variant if it can get called > > > in !atomic context too. > > > > For what? > > I'm thinking copy_code() should not use copy_from_user_nmi() if former > can be called in non-atomic context too. I understand, but why do you think this makes sense? Say, do you think it is fine to block in fuse_readpage() ? Anyway, this is off-topic and I won't argue. Oleg.
Re: [PATCH v2] x86/dumpstack: Fix misleading instruction pointer error message
On Tue, Nov 03, 2020 at 06:47:44PM +0100, Oleg Nesterov wrote: > > I'm thinking this should not use the atomic variant if it can get called > > in !atomic context too. > > For what? I'm thinking copy_code() should not use copy_from_user_nmi() if former can be called in non-atomic context too. -- Regards/Gruss, Boris. https://people.kernel.org/tglx/notes-about-netiquette
Re: [PATCH v2] x86/dumpstack: Fix misleading instruction pointer error message
On 11/03, Borislav Petkov wrote: > > On Tue, Nov 03, 2020 at 01:50:34PM +0100, Oleg Nesterov wrote: > > Another problem is that show_opcodes() makes no sense if user_mode(regs) > > and tsk is not current. > > Because if not current, we would access *some* user address space but > not the one to which regs belong to? Yes, because if not current, copy_from_user() will access the current's user address space at address = foreign process's regs->ip. > > Try "echo t > /proc/sysrq-trigger". > > What am I looking for? > > I see a bunch of: > > [ 37.622896] Code: Unable to access opcode bytes at RIP this means that foreign_regs->ip is not mmapped, > and three Code: lines with opcode bytes, as expected: > > [ 37.148693] Code: 11 0d 00 48 89 c6 4c 89 ef e8 98 07 00 00 48 83 f8 ff 0f > 84 3e 02 00 00 48 3b 05 b7 28 0d 00 48 89 c3 0f 83 b5 00 00 00 48 8b <0d> e7 > 10 0d 00 48 83 f8 0d 76 13 48 b8 28 75 6e 72 65 61 63 68 48 I'd say this is NOT expected and adds the unnecessary confusion. ./scripts/decodecode reports ... Code starting with the faulting instruction === 0: 0d e7 10 0d 00 or $0xd10e7,%eax 5: 48 83 f8 0d cmp$0xd,%rax 9: 76 13 jbe0x1e b: 48 b8 28 75 6e 72 65movabs $0x68636165726e7528,%rax 12: 61 63 68 15: 48 rex.W and this is because foreign_regs->ip happens to be a valid address in current->mm. > I'm thinking this should not use the atomic variant if it can get called > in !atomic context too. For what? Oleg.
Re: [PATCH v2] x86/dumpstack: Fix misleading instruction pointer error message
On Tue, Nov 03, 2020 at 01:50:34PM +0100, Oleg Nesterov wrote:
> Another problem is that show_opcodes() makes no sense if user_mode(regs)
> and tsk is not current.
Because if not current, we would access *some* user address space but
not the one to which regs belong to?
> Try "echo t > /proc/sysrq-trigger".
What am I looking for?
I see a bunch of:
[ 37.622896] Code: Unable to access opcode bytes at RIP
and three Code: lines with opcode bytes, as expected:
[ 37.148693] Code: 11 0d 00 48 89 c6 4c 89 ef e8 98 07 00 00 48 83 f8 ff 0f
84 3e 02 00 00 48 3b 05 b7 28 0d 00 48 89 c3 0f 83 b5 00 00 00 48 8b <0d> e7 10
0d 00 48 83 f8 0d 76 13 48 b8 28 75 6e 72 65 61 63 68 48
So all those other but the three cases, copy_code() failed.
> In this case copy_from_user_nmi() will either fail, or (worse) it will
> read the "random" memory from current->mm.
>
> Perhaps we can add something like
>
> if (user_mode(regs) && regs != task_pt_regs(current))
> return;
>
> at the start of show_opcodes() ?
tglx made it use copy_from_user_nmi() in:
d181d2da0141 ("x86/dumpstack: Dump user space code correctly again")
I'm thinking this should not use the atomic variant if it can get called
in !atomic context too.
Thomas?
Leaving in the rest for reference.
> > --- a/arch/x86/kernel/dumpstack.c
> > +++ b/arch/x86/kernel/dumpstack.c
> > @@ -115,7 +115,8 @@ void show_opcodes(struct pt_regs *regs, const char
> > *loglvl)
> > unsigned long prologue = regs->ip - PROLOGUE_SIZE;
> >
> > if (copy_code(regs, opcodes, prologue, sizeof(opcodes))) {
> > - printk("%sCode: Bad RIP value.\n", loglvl);
> > + printk("%sCode: Unable to access opcode bytes at RIP 0x%lx.\n",
> > + loglvl, prologue);
> > } else {
> > printk("%sCode: %" __stringify(PROLOGUE_SIZE) "ph <%02x> %"
> >__stringify(EPILOGUE_SIZE) "ph\n", loglvl, opcodes,
> > --
> > 2.25.1
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
Re: [PATCH v2] x86/dumpstack: Fix misleading instruction pointer error message
On 10/02, Mark Mossberg wrote:
>
> Printing "Bad RIP value" if copy_code() fails can be misleading for
> userspace pointers, since copy_code() can fail if the instruction
> pointer is valid, but the code is paged out.
Another problem is that show_opcodes() makes no sense if user_mode(regs)
and tsk is not current. Try "echo t > /proc/sysrq-trigger".
In this case copy_from_user_nmi() will either fail, or (worse) it will
read the "random" memory from current->mm.
Perhaps we can add something like
if (user_mode(regs) && regs != task_pt_regs(current))
return;
at the start of show_opcodes() ?
> --- a/arch/x86/kernel/dumpstack.c
> +++ b/arch/x86/kernel/dumpstack.c
> @@ -115,7 +115,8 @@ void show_opcodes(struct pt_regs *regs, const char
> *loglvl)
> unsigned long prologue = regs->ip - PROLOGUE_SIZE;
>
> if (copy_code(regs, opcodes, prologue, sizeof(opcodes))) {
> - printk("%sCode: Bad RIP value.\n", loglvl);
> + printk("%sCode: Unable to access opcode bytes at RIP 0x%lx.\n",
> +loglvl, prologue);
> } else {
> printk("%sCode: %" __stringify(PROLOGUE_SIZE) "ph <%02x> %"
> __stringify(EPILOGUE_SIZE) "ph\n", loglvl, opcodes,
> --
> 2.25.1
>
[PATCH v2] x86/dumpstack: Fix misleading instruction pointer error message
Printing "Bad RIP value" if copy_code() fails can be misleading for
userspace pointers, since copy_code() can fail if the instruction
pointer is valid, but the code is paged out. This is because copy_code()
calls copy_from_user_nmi() for userspace pointers, which disables page
fault handling.
This is reproducible in OOM situations, where it's plausible that the
code may be reclaimed in the time between entry into the kernel and when
this message is printed. This leaves a misleading log in dmesg that
suggests instruction pointer corruption has occurred, which may alarm
users.
This patch changes the message to state the error condition more
precisely.
Thanks to Jann Horn for help with understanding OOM reclamation.
Signed-off-by: Mark Mossberg
---
arch/x86/kernel/dumpstack.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/arch/x86/kernel/dumpstack.c b/arch/x86/kernel/dumpstack.c
index 48ce44576947..ea8d51ec251b 100644
--- a/arch/x86/kernel/dumpstack.c
+++ b/arch/x86/kernel/dumpstack.c
@@ -115,7 +115,8 @@ void show_opcodes(struct pt_regs *regs, const char *loglvl)
unsigned long prologue = regs->ip - PROLOGUE_SIZE;
if (copy_code(regs, opcodes, prologue, sizeof(opcodes))) {
- printk("%sCode: Bad RIP value.\n", loglvl);
+ printk("%sCode: Unable to access opcode bytes at RIP 0x%lx.\n",
+ loglvl, prologue);
} else {
printk("%sCode: %" __stringify(PROLOGUE_SIZE) "ph <%02x> %"
__stringify(EPILOGUE_SIZE) "ph\n", loglvl, opcodes,
--
2.25.1
