[v4,10/29] x86/die: Don't try to recover from an OOPS on a non-default stack
diff mbox

Message ID 37ac7589ff0ea147e8a21cda5eb84d3af1f6cd60.1466974736.git.luto@kernel.org
State New
Headers show

Commit Message

Andy Lutomirski June 26, 2016, 9:55 p.m. UTC
It's not going to work, because the scheduler will explode if we try
to schedule when running on an IST stack or similar.

This will matter when we let kernel stack overflows (which are #DF)
call die().

Signed-off-by: Andy Lutomirski <luto@kernel.org>
---
 arch/x86/kernel/dumpstack.c | 3 +++
 1 file changed, 3 insertions(+)

Comments

Borislav Petkov July 2, 2016, 5:24 p.m. UTC | #1
On Sun, Jun 26, 2016 at 02:55:32PM -0700, Andy Lutomirski wrote:
> It's not going to work, because the scheduler will explode if we try
> to schedule when running on an IST stack or similar.
> 
> This will matter when we let kernel stack overflows (which are #DF)
> call die().
> 
> Signed-off-by: Andy Lutomirski <luto@kernel.org>
> ---
>  arch/x86/kernel/dumpstack.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/arch/x86/kernel/dumpstack.c b/arch/x86/kernel/dumpstack.c
> index ef8017ca5ba9..352f022cfd5b 100644
> --- a/arch/x86/kernel/dumpstack.c
> +++ b/arch/x86/kernel/dumpstack.c
> @@ -245,6 +245,9 @@ void oops_end(unsigned long flags, struct pt_regs *regs, int signr)
>  		return;
>  	if (in_interrupt())
>  		panic("Fatal exception in interrupt");
> +	if (((current_stack_pointer() ^ (current_top_of_stack() - 1))
> +	     & ~(THREAD_SIZE - 1)) != 0)

Ugh, that's hard to parse. You could remove the "!= 0" at least to
shorten it a bit and have one less braces level.

Or maybe even do something like that to make it a bit more readable:

        if ((current_stack_pointer() ^ (current_top_of_stack() - 1))
                        &
             ~(THREAD_SIZE - 1))
                panic("Fatal exception on non-default stack");

Meh.

> +		panic("Fatal exception on special stack");

			"Fatal exception on non-default stack"

maybe?
Josh Poimboeuf July 2, 2016, 6:34 p.m. UTC | #2
On Sat, Jul 02, 2016 at 07:24:41PM +0200, Borislav Petkov wrote:
> On Sun, Jun 26, 2016 at 02:55:32PM -0700, Andy Lutomirski wrote:
> > It's not going to work, because the scheduler will explode if we try
> > to schedule when running on an IST stack or similar.
> > 
> > This will matter when we let kernel stack overflows (which are #DF)
> > call die().
> > 
> > Signed-off-by: Andy Lutomirski <luto@kernel.org>
> > ---
> >  arch/x86/kernel/dumpstack.c | 3 +++
> >  1 file changed, 3 insertions(+)
> > 
> > diff --git a/arch/x86/kernel/dumpstack.c b/arch/x86/kernel/dumpstack.c
> > index ef8017ca5ba9..352f022cfd5b 100644
> > --- a/arch/x86/kernel/dumpstack.c
> > +++ b/arch/x86/kernel/dumpstack.c
> > @@ -245,6 +245,9 @@ void oops_end(unsigned long flags, struct pt_regs *regs, int signr)
> >  		return;
> >  	if (in_interrupt())
> >  		panic("Fatal exception in interrupt");
> > +	if (((current_stack_pointer() ^ (current_top_of_stack() - 1))
> > +	     & ~(THREAD_SIZE - 1)) != 0)
> 
> Ugh, that's hard to parse. You could remove the "!= 0" at least to
> shorten it a bit and have one less braces level.
> 
> Or maybe even do something like that to make it a bit more readable:
> 
>         if ((current_stack_pointer() ^ (current_top_of_stack() - 1))
>                         &
>              ~(THREAD_SIZE - 1))
>                 panic("Fatal exception on non-default stack");
> 
> Meh.

A helper function would be even better.

The existing 'object_is_on_stack()' can probably be used:

	if (!object_is_on_stack(current_top_of_stack()))
		panic("...");

Though that function isn't quite accurately named.  It should really
have 'task_stack' in its name, like 'object_is_on_task_stack()'.  Or
even better, something more concise like 'on_task_stack()'.
Borislav Petkov July 3, 2016, 9:40 a.m. UTC | #3
On Sat, Jul 02, 2016 at 01:34:51PM -0500, Josh Poimboeuf wrote:
> The existing 'object_is_on_stack()' can probably be used:
> 
> 	if (!object_is_on_stack(current_top_of_stack()))
> 		panic("...");
> 
> Though that function isn't quite accurately named.  It should really
> have 'task_stack' in its name, like 'object_is_on_task_stack()'.  Or
> even better, something more concise like 'on_task_stack()'.

So I'm obviously missing something here:

object_is_on_stack() uses task_stack_page(current) -> task_struct.stack
while current_stack_pointer() reads %rsp directly.

I'm guessing %rsp and task_struct.stack are in sync?
Andy Lutomirski July 3, 2016, 2:25 p.m. UTC | #4
On Sat, Jul 2, 2016 at 11:34 AM, Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> On Sat, Jul 02, 2016 at 07:24:41PM +0200, Borislav Petkov wrote:
>> On Sun, Jun 26, 2016 at 02:55:32PM -0700, Andy Lutomirski wrote:
>> > It's not going to work, because the scheduler will explode if we try
>> > to schedule when running on an IST stack or similar.
>> >
>> > This will matter when we let kernel stack overflows (which are #DF)
>> > call die().
>> >
>> > Signed-off-by: Andy Lutomirski <luto@kernel.org>
>> > ---
>> >  arch/x86/kernel/dumpstack.c | 3 +++
>> >  1 file changed, 3 insertions(+)
>> >
>> > diff --git a/arch/x86/kernel/dumpstack.c b/arch/x86/kernel/dumpstack.c
>> > index ef8017ca5ba9..352f022cfd5b 100644
>> > --- a/arch/x86/kernel/dumpstack.c
>> > +++ b/arch/x86/kernel/dumpstack.c
>> > @@ -245,6 +245,9 @@ void oops_end(unsigned long flags, struct pt_regs *regs, int signr)
>> >             return;
>> >     if (in_interrupt())
>> >             panic("Fatal exception in interrupt");
>> > +   if (((current_stack_pointer() ^ (current_top_of_stack() - 1))
>> > +        & ~(THREAD_SIZE - 1)) != 0)
>>
>> Ugh, that's hard to parse. You could remove the "!= 0" at least to
>> shorten it a bit and have one less braces level.
>>
>> Or maybe even do something like that to make it a bit more readable:
>>
>>         if ((current_stack_pointer() ^ (current_top_of_stack() - 1))
>>                         &
>>              ~(THREAD_SIZE - 1))
>>                 panic("Fatal exception on non-default stack");
>>
>> Meh.
>
> A helper function would be even better.
>
> The existing 'object_is_on_stack()' can probably be used:
>
>         if (!object_is_on_stack(current_top_of_stack()))
>                 panic("...");
>
> Though that function isn't quite accurately named.  It should really
> have 'task_stack' in its name, like 'object_is_on_task_stack()'.  Or
> even better, something more concise like 'on_task_stack()'.
>

Given that the very next patch deletes this code, I vote for leaving
it alone.  Or I could fold the patches together.

--Andy
Borislav Petkov July 3, 2016, 6:42 p.m. UTC | #5
On Sun, Jul 03, 2016 at 07:25:05AM -0700, Andy Lutomirski wrote:
> Given that the very next patch deletes this code, I vote for leaving
> it alone.  Or I could fold the patches together.

Ah, true. Yes, please fold them together.

Patch
diff mbox

diff --git a/arch/x86/kernel/dumpstack.c b/arch/x86/kernel/dumpstack.c
index ef8017ca5ba9..352f022cfd5b 100644
--- a/arch/x86/kernel/dumpstack.c
+++ b/arch/x86/kernel/dumpstack.c
@@ -245,6 +245,9 @@  void oops_end(unsigned long flags, struct pt_regs *regs, int signr)
 		return;
 	if (in_interrupt())
 		panic("Fatal exception in interrupt");
+	if (((current_stack_pointer() ^ (current_top_of_stack() - 1))
+	     & ~(THREAD_SIZE - 1)) != 0)
+		panic("Fatal exception on special stack");
 	if (panic_on_oops)
 		panic("Fatal exception");
 	do_exit(signr);