[PATCH] powerpc: Don't print kernel instructions in show_user_instructions()

[PATCH] powerpc: Don't print kernel instructions in show_user_instructions()

[Michael Ellerman]

Recently we implemented show_user_instructions() which dumps the code
around the NIP when a user space process dies with an unhandled
signal. This was modelled on the x86 code, and we even went so far as
to implement the exact same bug, namely that if the user process
crashed with its NIP pointing into the kernel we will dump kernel text
to dmesg. eg:

  bad-bctr[2996]: segfault (11) at c000000000010000 nip c000000000010000 lr 12d0b0894 code 1
  bad-bctr[2996]: code: fbe10068 7cbe2b78 7c7f1b78 fb610048 38a10028 38810020 fb810050 7f8802a6
  bad-bctr[2996]: code: 3860001c f8010080 48242371 60000000 <7c7b1b79> 4082002c e8010080 eb610048

This was discovered on x86 by Jann Horn and fixed in commit
342db04ae712 ("x86/dumpstack: Don't dump kernel memory based on usermode RIP").

Fix it by checking the adjusted NIP value (pc) and number of
instructions against USER_DS, and bail if we fail the check, eg:

  bad-bctr[2969]: segfault (11) at c000000000010000 nip c000000000010000 lr 107930894 code 1
  bad-bctr[2969]: Bad NIP, not dumping instructions.

Fixes: 88b0fe175735 ("powerpc: Add show_user_instructions()")
Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
---
 arch/powerpc/kernel/process.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.c
index 913c5725cdb2..bb6ac471a784 100644
--- a/arch/powerpc/kernel/process.c
+++ b/arch/powerpc/kernel/process.c
@@ -1306,6 +1306,16 @@ void show_user_instructions(struct pt_regs *regs)
 
 	pc = regs->nip - (instructions_to_print * 3 / 4 * sizeof(int));
 
+	/*
+	 * Make sure the NIP points at userspace, not kernel text/data or
+	 * elsewhere.
+	 */
+	if (!__access_ok(pc, instructions_to_print * sizeof(int), USER_DS)) {
+		pr_info("%s[%d]: Bad NIP, not dumping instructions.\n",
+			current->comm, current->pid);
+		return;
+	}
+
 	pr_info("%s[%d]: code: ", current->comm, current->pid);
 
 	for (i = 0; i < instructions_to_print; i++) {
-- 
2.17.1

Re: [PATCH] powerpc: Don't print kernel instructions in show_user_instructions()

[Christophe LEROY]

Le 05/10/2018 à 15:21, Michael Ellerman a écrit :
> Recently we implemented show_user_instructions() which dumps the code
> around the NIP when a user space process dies with an unhandled
> signal. This was modelled on the x86 code, and we even went so far as
> to implement the exact same bug, namely that if the user process
> crashed with its NIP pointing into the kernel we will dump kernel text
> to dmesg. eg:
> 
>    bad-bctr[2996]: segfault (11) at c000000000010000 nip c000000000010000 lr 12d0b0894 code 1
>    bad-bctr[2996]: code: fbe10068 7cbe2b78 7c7f1b78 fb610048 38a10028 38810020 fb810050 7f8802a6
>    bad-bctr[2996]: code: 3860001c f8010080 48242371 60000000 <7c7b1b79> 4082002c e8010080 eb610048
> 
> This was discovered on x86 by Jann Horn and fixed in commit
> 342db04ae712 ("x86/dumpstack: Don't dump kernel memory based on usermode RIP").
> 
> Fix it by checking the adjusted NIP value (pc) and number of
> instructions against USER_DS, and bail if we fail the check, eg:
> 
>    bad-bctr[2969]: segfault (11) at c000000000010000 nip c000000000010000 lr 107930894 code 1
>    bad-bctr[2969]: Bad NIP, not dumping instructions.
> 
> Fixes: 88b0fe175735 ("powerpc: Add show_user_instructions()")
> Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
> ---
>   arch/powerpc/kernel/process.c | 10 ++++++++++
>   1 file changed, 10 insertions(+)
> 
> diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.c
> index 913c5725cdb2..bb6ac471a784 100644
> --- a/arch/powerpc/kernel/process.c
> +++ b/arch/powerpc/kernel/process.c
> @@ -1306,6 +1306,16 @@ void show_user_instructions(struct pt_regs *regs)
>   
>   	pc = regs->nip - (instructions_to_print * 3 / 4 * sizeof(int));
>   
> +	/*
> +	 * Make sure the NIP points at userspace, not kernel text/data or
> +	 * elsewhere.
> +	 */
> +	if (!__access_ok(pc, instructions_to_print * sizeof(int), USER_DS)) {
> +		pr_info("%s[%d]: Bad NIP, not dumping instructions.\n",
> +			current->comm, current->pid);
> +		return;
> +	}
> +

This will conflict with my serie 
https://patchwork.ozlabs.org/project/linuxppc-dev/list/?series=64611 
which changes instructions_to_print to a constant. Will you merge it or 
do you expect me to rebase my serie ?

Reviewed-by: Christophe Leroy <christophe.leroy@c-s.fr>

Christophe

>   	pr_info("%s[%d]: code: ", current->comm, current->pid);
>   
>   	for (i = 0; i < instructions_to_print; i++) {
> 

Re: [PATCH] powerpc: Don't print kernel instructions in show_user_instructions()

[Michael Ellerman]

Christophe LEROY <christophe.leroy@c-s.fr> writes:
> Le 05/10/2018 à 15:21, Michael Ellerman a écrit :
>> Recently we implemented show_user_instructions() which dumps the code
...
>> diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.c
>> index 913c5725cdb2..bb6ac471a784 100644
>> --- a/arch/powerpc/kernel/process.c
>> +++ b/arch/powerpc/kernel/process.c
>> @@ -1306,6 +1306,16 @@ void show_user_instructions(struct pt_regs *regs)
>>   
>>   	pc = regs->nip - (instructions_to_print * 3 / 4 * sizeof(int));
>>   
>> +	/*
>> +	 * Make sure the NIP points at userspace, not kernel text/data or
>> +	 * elsewhere.
>> +	 */
>> +	if (!__access_ok(pc, instructions_to_print * sizeof(int), USER_DS)) {
>> +		pr_info("%s[%d]: Bad NIP, not dumping instructions.\n",
>> +			current->comm, current->pid);
>> +		return;
>> +	}
>> +
>
> This will conflict with my serie 
> https://patchwork.ozlabs.org/project/linuxppc-dev/list/?series=64611 
> which changes instructions_to_print to a constant. Will you merge it or 
> do you expect me to rebase my serie ?

I can fix it up.

But I see you've already rebased it and resent, you're too quick for me :)

cheers

Re: [PATCH] powerpc: Don't print kernel instructions in show_user_instructions()

[Jann Horn]

On Fri, Oct 5, 2018 at 3:21 PM Michael Ellerman <mpe@ellerman.id.au> wrote:
> Recently we implemented show_user_instructions() which dumps the code
> around the NIP when a user space process dies with an unhandled
> signal. This was modelled on the x86 code, and we even went so far as
> to implement the exact same bug, namely that if the user process
> crashed with its NIP pointing into the kernel we will dump kernel text
> to dmesg. eg:
>
>   bad-bctr[2996]: segfault (11) at c000000000010000 nip c000000000010000 lr 12d0b0894 code 1
>   bad-bctr[2996]: code: fbe10068 7cbe2b78 7c7f1b78 fb610048 38a10028 38810020 fb810050 7f8802a6
>   bad-bctr[2996]: code: 3860001c f8010080 48242371 60000000 <7c7b1b79> 4082002c e8010080 eb610048
>
> This was discovered on x86 by Jann Horn and fixed in commit
> 342db04ae712 ("x86/dumpstack: Don't dump kernel memory based on usermode RIP").
>
> Fix it by checking the adjusted NIP value (pc) and number of
> instructions against USER_DS, and bail if we fail the check, eg:

This fix looks good to me.

In the long term, I think it is somewhat awkward to use
probe_kernel_address(), which uses set_fs(KERNEL_DS), when you
actually just want to access userspace memory. It might make sense to
provide a better helper for explicitly accessing memory with USER_DS.

Re: [PATCH] powerpc: Don't print kernel instructions in show_user_instructions()

[Michael Ellerman]

Jann Horn <jannh@google.com> writes:
> On Fri, Oct 5, 2018 at 3:21 PM Michael Ellerman <mpe@ellerman.id.au> wrote:
>> Recently we implemented show_user_instructions() which dumps the code
>> around the NIP when a user space process dies with an unhandled
>> signal. This was modelled on the x86 code, and we even went so far as
>> to implement the exact same bug, namely that if the user process
>> crashed with its NIP pointing into the kernel we will dump kernel text
>> to dmesg. eg:
>>
>>   bad-bctr[2996]: segfault (11) at c000000000010000 nip c000000000010000 lr 12d0b0894 code 1
>>   bad-bctr[2996]: code: fbe10068 7cbe2b78 7c7f1b78 fb610048 38a10028 38810020 fb810050 7f8802a6
>>   bad-bctr[2996]: code: 3860001c f8010080 48242371 60000000 <7c7b1b79> 4082002c e8010080 eb610048
>>
>> This was discovered on x86 by Jann Horn and fixed in commit
>> 342db04ae712 ("x86/dumpstack: Don't dump kernel memory based on usermode RIP").
>>
>> Fix it by checking the adjusted NIP value (pc) and number of
>> instructions against USER_DS, and bail if we fail the check, eg:
>
> This fix looks good to me.

Thanks.

> In the long term, I think it is somewhat awkward to use
> probe_kernel_address(), which uses set_fs(KERNEL_DS), when you
> actually just want to access userspace memory. It might make sense to
> provide a better helper for explicitly accessing memory with USER_DS.

Yes I agree, it's a bit messy. A probe_user_read() that sets USER_DS and
does the access_ok() check would be less error prone I think.

cheers

Re: [PATCH] powerpc: Don't print kernel instructions in show_user_instructions()

[Murilo Opsfelder Araujo]

On Fri, Oct 05, 2018 at 11:21:23PM +1000, Michael Ellerman wrote:
> Recently we implemented show_user_instructions() which dumps the code
> around the NIP when a user space process dies with an unhandled
> signal. This was modelled on the x86 code, and we even went so far as
> to implement the exact same bug, namely that if the user process
> crashed with its NIP pointing into the kernel we will dump kernel text
> to dmesg. eg:
>
>   bad-bctr[2996]: segfault (11) at c000000000010000 nip c000000000010000 lr 12d0b0894 code 1
>   bad-bctr[2996]: code: fbe10068 7cbe2b78 7c7f1b78 fb610048 38a10028 38810020 fb810050 7f8802a6
>   bad-bctr[2996]: code: 3860001c f8010080 48242371 60000000 <7c7b1b79> 4082002c e8010080 eb610048
>
> This was discovered on x86 by Jann Horn and fixed in commit
> 342db04ae712 ("x86/dumpstack: Don't dump kernel memory based on usermode RIP").
>
> Fix it by checking the adjusted NIP value (pc) and number of
> instructions against USER_DS, and bail if we fail the check, eg:
>
>   bad-bctr[2969]: segfault (11) at c000000000010000 nip c000000000010000 lr 107930894 code 1
>   bad-bctr[2969]: Bad NIP, not dumping instructions.
>
> Fixes: 88b0fe175735 ("powerpc: Add show_user_instructions()")
> Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>

Reviewed-by: Murilo Opsfelder Araujo <muriloo@linux.ibm.com>

Thank you all!

> ---
>  arch/powerpc/kernel/process.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
>
> diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.c
> index 913c5725cdb2..bb6ac471a784 100644
> --- a/arch/powerpc/kernel/process.c
> +++ b/arch/powerpc/kernel/process.c
> @@ -1306,6 +1306,16 @@ void show_user_instructions(struct pt_regs *regs)
>
>  	pc = regs->nip - (instructions_to_print * 3 / 4 * sizeof(int));
>
> +	/*
> +	 * Make sure the NIP points at userspace, not kernel text/data or
> +	 * elsewhere.
> +	 */
> +	if (!__access_ok(pc, instructions_to_print * sizeof(int), USER_DS)) {
> +		pr_info("%s[%d]: Bad NIP, not dumping instructions.\n",
> +			current->comm, current->pid);
> +		return;
> +	}
> +
>  	pr_info("%s[%d]: code: ", current->comm, current->pid);
>
>  	for (i = 0; i < instructions_to_print; i++) {
> --
> 2.17.1
>

--
Murilo

Re: powerpc: Don't print kernel instructions in show_user_instructions()

[Michael Ellerman]

On Fri, 2018-10-05 at 13:21:23 UTC, Michael Ellerman wrote:
> Recently we implemented show_user_instructions() which dumps the code
> around the NIP when a user space process dies with an unhandled
> signal. This was modelled on the x86 code, and we even went so far as
> to implement the exact same bug, namely that if the user process
> crashed with its NIP pointing into the kernel we will dump kernel text
> to dmesg. eg:
> 
>   bad-bctr[2996]: segfault (11) at c000000000010000 nip c000000000010000 lr 12d0b0894 code 1
>   bad-bctr[2996]: code: fbe10068 7cbe2b78 7c7f1b78 fb610048 38a10028 38810020 fb810050 7f8802a6
>   bad-bctr[2996]: code: 3860001c f8010080 48242371 60000000 <7c7b1b79> 4082002c e8010080 eb610048
> 
> This was discovered on x86 by Jann Horn and fixed in commit
> 342db04ae712 ("x86/dumpstack: Don't dump kernel memory based on usermode RIP").
> 
> Fix it by checking the adjusted NIP value (pc) and number of
> instructions against USER_DS, and bail if we fail the check, eg:
> 
>   bad-bctr[2969]: segfault (11) at c000000000010000 nip c000000000010000 lr 107930894 code 1
>   bad-bctr[2969]: Bad NIP, not dumping instructions.
> 
> Fixes: 88b0fe175735 ("powerpc: Add show_user_instructions()")
> Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
> Reviewed-by: Christophe Leroy <christophe.leroy@c-s.fr>
> Reviewed-by: Murilo Opsfelder Araujo <muriloo@linux.ibm.com>

Applied to powerpc fixes.

https://git.kernel.org/powerpc/c/a932ed3b718147c6537da290b7a91e

cheers

Re: [PATCH] powerpc: Don't print kernel instructions in show_user_instructions()

[Christophe LEROY]

Le 05/10/2018 à 15:21, Michael Ellerman a écrit :
> Recently we implemented show_user_instructions() which dumps the code
> around the NIP when a user space process dies with an unhandled
> signal. This was modelled on the x86 code, and we even went so far as
> to implement the exact same bug, namely that if the user process
> crashed with its NIP pointing into the kernel we will dump kernel text
> to dmesg. eg:
> 
>    bad-bctr[2996]: segfault (11) at c000000000010000 nip c000000000010000 lr 12d0b0894 code 1
>    bad-bctr[2996]: code: fbe10068 7cbe2b78 7c7f1b78 fb610048 38a10028 38810020 fb810050 7f8802a6
>    bad-bctr[2996]: code: 3860001c f8010080 48242371 60000000 <7c7b1b79> 4082002c e8010080 eb610048
> 
> This was discovered on x86 by Jann Horn and fixed in commit
> 342db04ae712 ("x86/dumpstack: Don't dump kernel memory based on usermode RIP").
> 
> Fix it by checking the adjusted NIP value (pc) and number of
> instructions against USER_DS, and bail if we fail the check, eg:
> 
>    bad-bctr[2969]: segfault (11) at c000000000010000 nip c000000000010000 lr 107930894 code 1
>    bad-bctr[2969]: Bad NIP, not dumping instructions.
> 
> Fixes: 88b0fe175735 ("powerpc: Add show_user_instructions()")
> Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
> ---
>   arch/powerpc/kernel/process.c | 10 ++++++++++
>   1 file changed, 10 insertions(+)
> 
> diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.c
> index 913c5725cdb2..bb6ac471a784 100644
> --- a/arch/powerpc/kernel/process.c
> +++ b/arch/powerpc/kernel/process.c
> @@ -1306,6 +1306,16 @@ void show_user_instructions(struct pt_regs *regs)
>   
>   	pc = regs->nip - (instructions_to_print * 3 / 4 * sizeof(int));
>   
> +	/*
> +	 * Make sure the NIP points at userspace, not kernel text/data or
> +	 * elsewhere.
> +	 */
> +	if (!__access_ok(pc, instructions_to_print * sizeof(int), USER_DS)) {
> +		pr_info("%s[%d]: Bad NIP, not dumping instructions.\n",
> +			current->comm, current->pid);
> +		return;
> +	}
> +

Is there any reason for not using access_ok() here ?

Christophe

>   	pr_info("%s[%d]: code: ", current->comm, current->pid);
>   
>   	for (i = 0; i < instructions_to_print; i++) {
> 

Re: [PATCH] powerpc: Don't print kernel instructions in show_user_instructions()

[Jann Horn]

On Thu, Oct 18, 2018 at 11:28 AM Christophe LEROY
<christophe.leroy@c-s.fr> wrote:
> Le 05/10/2018 à 15:21, Michael Ellerman a écrit :
> > Recently we implemented show_user_instructions() which dumps the code
> > around the NIP when a user space process dies with an unhandled
> > signal. This was modelled on the x86 code, and we even went so far as
> > to implement the exact same bug, namely that if the user process
> > crashed with its NIP pointing into the kernel we will dump kernel text
> > to dmesg. eg:
> >
> >    bad-bctr[2996]: segfault (11) at c000000000010000 nip c000000000010000 lr 12d0b0894 code 1
> >    bad-bctr[2996]: code: fbe10068 7cbe2b78 7c7f1b78 fb610048 38a10028 38810020 fb810050 7f8802a6
> >    bad-bctr[2996]: code: 3860001c f8010080 48242371 60000000 <7c7b1b79> 4082002c e8010080 eb610048
> >
> > This was discovered on x86 by Jann Horn and fixed in commit
> > 342db04ae712 ("x86/dumpstack: Don't dump kernel memory based on usermode RIP").
> >
> > Fix it by checking the adjusted NIP value (pc) and number of
> > instructions against USER_DS, and bail if we fail the check, eg:
> >
> >    bad-bctr[2969]: segfault (11) at c000000000010000 nip c000000000010000 lr 107930894 code 1
> >    bad-bctr[2969]: Bad NIP, not dumping instructions.
> >
> > Fixes: 88b0fe175735 ("powerpc: Add show_user_instructions()")
> > Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
> > ---
> >   arch/powerpc/kernel/process.c | 10 ++++++++++
> >   1 file changed, 10 insertions(+)
> >
> > diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.c
> > index 913c5725cdb2..bb6ac471a784 100644
> > --- a/arch/powerpc/kernel/process.c
> > +++ b/arch/powerpc/kernel/process.c
> > @@ -1306,6 +1306,16 @@ void show_user_instructions(struct pt_regs *regs)
> >
> >       pc = regs->nip - (instructions_to_print * 3 / 4 * sizeof(int));
> >
> > +     /*
> > +      * Make sure the NIP points at userspace, not kernel text/data or
> > +      * elsewhere.
> > +      */
> > +     if (!__access_ok(pc, instructions_to_print * sizeof(int), USER_DS)) {
> > +             pr_info("%s[%d]: Bad NIP, not dumping instructions.\n",
> > +                     current->comm, current->pid);
> > +             return;
> > +     }
> > +
>
> Is there any reason for not using access_ok() here ?

It's probably more robust this way, in case someone decides to call
into this from kernel exception context at some point, or something
like that?

Re: [PATCH] powerpc: Don't print kernel instructions in show_user_instructions()

[Christophe LEROY]

Le 18/10/2018 à 13:12, Jann Horn a écrit :
> On Thu, Oct 18, 2018 at 11:28 AM Christophe LEROY
> <christophe.leroy@c-s.fr> wrote:
>> Le 05/10/2018 à 15:21, Michael Ellerman a écrit :
>>> Recently we implemented show_user_instructions() which dumps the code
>>> around the NIP when a user space process dies with an unhandled
>>> signal. This was modelled on the x86 code, and we even went so far as
>>> to implement the exact same bug, namely that if the user process
>>> crashed with its NIP pointing into the kernel we will dump kernel text
>>> to dmesg. eg:
>>>
>>>     bad-bctr[2996]: segfault (11) at c000000000010000 nip c000000000010000 lr 12d0b0894 code 1
>>>     bad-bctr[2996]: code: fbe10068 7cbe2b78 7c7f1b78 fb610048 38a10028 38810020 fb810050 7f8802a6
>>>     bad-bctr[2996]: code: 3860001c f8010080 48242371 60000000 <7c7b1b79> 4082002c e8010080 eb610048
>>>
>>> This was discovered on x86 by Jann Horn and fixed in commit
>>> 342db04ae712 ("x86/dumpstack: Don't dump kernel memory based on usermode RIP").
>>>
>>> Fix it by checking the adjusted NIP value (pc) and number of
>>> instructions against USER_DS, and bail if we fail the check, eg:
>>>
>>>     bad-bctr[2969]: segfault (11) at c000000000010000 nip c000000000010000 lr 107930894 code 1
>>>     bad-bctr[2969]: Bad NIP, not dumping instructions.
>>>
>>> Fixes: 88b0fe175735 ("powerpc: Add show_user_instructions()")
>>> Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
>>> ---
>>>    arch/powerpc/kernel/process.c | 10 ++++++++++
>>>    1 file changed, 10 insertions(+)
>>>
>>> diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.c
>>> index 913c5725cdb2..bb6ac471a784 100644
>>> --- a/arch/powerpc/kernel/process.c
>>> +++ b/arch/powerpc/kernel/process.c
>>> @@ -1306,6 +1306,16 @@ void show_user_instructions(struct pt_regs *regs)
>>>
>>>        pc = regs->nip - (instructions_to_print * 3 / 4 * sizeof(int));
>>>
>>> +     /*
>>> +      * Make sure the NIP points at userspace, not kernel text/data or
>>> +      * elsewhere.
>>> +      */
>>> +     if (!__access_ok(pc, instructions_to_print * sizeof(int), USER_DS)) {
>>> +             pr_info("%s[%d]: Bad NIP, not dumping instructions.\n",
>>> +                     current->comm, current->pid);
>>> +             return;
>>> +     }
>>> +
>>
>> Is there any reason for not using access_ok() here ?
> 
> It's probably more robust this way, in case someone decides to call
> into this from kernel exception context at some point, or something
> like that?
> 

But access_ok() uses current->thread.addr_limit, while USER_DS may 
provide a larger segment:

#ifdef __powerpc64__
/* We use TASK_SIZE_USER64 as TASK_SIZE is not constant */
#define USER_DS		MAKE_MM_SEG(TASK_SIZE_USER64 - 1)
#else
#define USER_DS		MAKE_MM_SEG(TASK_SIZE - 1)
#endif

Christophe

Re: [PATCH] powerpc: Don't print kernel instructions in show_user_instructions()

[Jann Horn]

+cc x86 folks

On Thu, Oct 18, 2018 at 1:18 PM Christophe LEROY
<christophe.leroy@c-s.fr> wrote:
> Le 18/10/2018 à 13:12, Jann Horn a écrit :
> > On Thu, Oct 18, 2018 at 11:28 AM Christophe LEROY
> > <christophe.leroy@c-s.fr> wrote:
> >> Le 05/10/2018 à 15:21, Michael Ellerman a écrit :
> >>> Recently we implemented show_user_instructions() which dumps the code
> >>> around the NIP when a user space process dies with an unhandled
> >>> signal. This was modelled on the x86 code, and we even went so far as
> >>> to implement the exact same bug, namely that if the user process
> >>> crashed with its NIP pointing into the kernel we will dump kernel text
> >>> to dmesg. eg:
> >>>
> >>>     bad-bctr[2996]: segfault (11) at c000000000010000 nip c000000000010000 lr 12d0b0894 code 1
> >>>     bad-bctr[2996]: code: fbe10068 7cbe2b78 7c7f1b78 fb610048 38a10028 38810020 fb810050 7f8802a6
> >>>     bad-bctr[2996]: code: 3860001c f8010080 48242371 60000000 <7c7b1b79> 4082002c e8010080 eb610048
> >>>
> >>> This was discovered on x86 by Jann Horn and fixed in commit
> >>> 342db04ae712 ("x86/dumpstack: Don't dump kernel memory based on usermode RIP").
> >>>
> >>> Fix it by checking the adjusted NIP value (pc) and number of
> >>> instructions against USER_DS, and bail if we fail the check, eg:
> >>>
> >>>     bad-bctr[2969]: segfault (11) at c000000000010000 nip c000000000010000 lr 107930894 code 1
> >>>     bad-bctr[2969]: Bad NIP, not dumping instructions.
> >>>
> >>> Fixes: 88b0fe175735 ("powerpc: Add show_user_instructions()")
> >>> Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
> >>> ---
> >>>    arch/powerpc/kernel/process.c | 10 ++++++++++
> >>>    1 file changed, 10 insertions(+)
> >>>
> >>> diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.c
> >>> index 913c5725cdb2..bb6ac471a784 100644
> >>> --- a/arch/powerpc/kernel/process.c
> >>> +++ b/arch/powerpc/kernel/process.c
> >>> @@ -1306,6 +1306,16 @@ void show_user_instructions(struct pt_regs *regs)
> >>>
> >>>        pc = regs->nip - (instructions_to_print * 3 / 4 * sizeof(int));
> >>>
> >>> +     /*
> >>> +      * Make sure the NIP points at userspace, not kernel text/data or
> >>> +      * elsewhere.
> >>> +      */
> >>> +     if (!__access_ok(pc, instructions_to_print * sizeof(int), USER_DS)) {
> >>> +             pr_info("%s[%d]: Bad NIP, not dumping instructions.\n",
> >>> +                     current->comm, current->pid);
> >>> +             return;
> >>> +     }
> >>> +
> >>
> >> Is there any reason for not using access_ok() here ?
> >
> > It's probably more robust this way, in case someone decides to call
> > into this from kernel exception context at some point, or something
> > like that?
> >
>
> But access_ok() uses current->thread.addr_limit, while USER_DS may
> provide a larger segment:
>
> #ifdef __powerpc64__
> /* We use TASK_SIZE_USER64 as TASK_SIZE is not constant */
> #define USER_DS         MAKE_MM_SEG(TASK_SIZE_USER64 - 1)
> #else
> #define USER_DS         MAKE_MM_SEG(TASK_SIZE - 1)
> #endif

Where do you write a smaller value than USER_DS into the addr_limit?

The kernel is full of places that assume that any access up to USER_DS
is safe; for example, perf_output_sample_ustack(),
get_perf_callchain(), do_exit(), flush_old_exec(), vma_dump_size(),
... - and I also don't see anything in the powerpc code that would
ever write a smaller value into the addr_limit.

I don't know powerpc well, but AFAIK the rule on X86 is basically that
even for compat tasks, attempting to access anything up to USER_DS is
safe because the kernel doesn't put any kernel mappings there. Is that
different on powerpc?

Re: [PATCH] powerpc: Don't print kernel instructions in show_user_instructions()

[Michael Ellerman]

Jann Horn <jannh@google.com> writes:
> +cc x86 folks
>
> On Thu, Oct 18, 2018 at 1:18 PM Christophe LEROY
> <christophe.leroy@c-s.fr> wrote:
>> Le 18/10/2018 à 13:12, Jann Horn a écrit :
>> > On Thu, Oct 18, 2018 at 11:28 AM Christophe LEROY
>> > <christophe.leroy@c-s.fr> wrote:
>> >> Le 05/10/2018 à 15:21, Michael Ellerman a écrit :
>> >>> Recently we implemented show_user_instructions() which dumps the code
>> >>> around the NIP when a user space process dies with an unhandled
>> >>> signal. This was modelled on the x86 code, and we even went so far as
>> >>> to implement the exact same bug, namely that if the user process
>> >>> crashed with its NIP pointing into the kernel we will dump kernel text
>> >>> to dmesg. eg:
>> >>>
>> >>>     bad-bctr[2996]: segfault (11) at c000000000010000 nip c000000000010000 lr 12d0b0894 code 1
>> >>>     bad-bctr[2996]: code: fbe10068 7cbe2b78 7c7f1b78 fb610048 38a10028 38810020 fb810050 7f8802a6
>> >>>     bad-bctr[2996]: code: 3860001c f8010080 48242371 60000000 <7c7b1b79> 4082002c e8010080 eb610048
>> >>>
>> >>> This was discovered on x86 by Jann Horn and fixed in commit
>> >>> 342db04ae712 ("x86/dumpstack: Don't dump kernel memory based on usermode RIP").
>> >>>
>> >>> Fix it by checking the adjusted NIP value (pc) and number of
>> >>> instructions against USER_DS, and bail if we fail the check, eg:
>> >>>
>> >>>     bad-bctr[2969]: segfault (11) at c000000000010000 nip c000000000010000 lr 107930894 code 1
>> >>>     bad-bctr[2969]: Bad NIP, not dumping instructions.
>> >>>
>> >>> Fixes: 88b0fe175735 ("powerpc: Add show_user_instructions()")
>> >>> Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
>> >>> ---
>> >>>    arch/powerpc/kernel/process.c | 10 ++++++++++
>> >>>    1 file changed, 10 insertions(+)
>> >>>
>> >>> diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.c
>> >>> index 913c5725cdb2..bb6ac471a784 100644
>> >>> --- a/arch/powerpc/kernel/process.c
>> >>> +++ b/arch/powerpc/kernel/process.c
>> >>> @@ -1306,6 +1306,16 @@ void show_user_instructions(struct pt_regs *regs)
>> >>>
>> >>>        pc = regs->nip - (instructions_to_print * 3 / 4 * sizeof(int));
>> >>>
>> >>> +     /*
>> >>> +      * Make sure the NIP points at userspace, not kernel text/data or
>> >>> +      * elsewhere.
>> >>> +      */
>> >>> +     if (!__access_ok(pc, instructions_to_print * sizeof(int), USER_DS)) {
>> >>> +             pr_info("%s[%d]: Bad NIP, not dumping instructions.\n",
>> >>> +                     current->comm, current->pid);
>> >>> +             return;
>> >>> +     }
>> >>> +
>> >>
>> >> Is there any reason for not using access_ok() here ?
>> >
>> > It's probably more robust this way, in case someone decides to call
>> > into this from kernel exception context at some point, or something
>> > like that?
>> >
>>
>> But access_ok() uses current->thread.addr_limit, while USER_DS may
>> provide a larger segment:
>>
>> #ifdef __powerpc64__
>> /* We use TASK_SIZE_USER64 as TASK_SIZE is not constant */
>> #define USER_DS         MAKE_MM_SEG(TASK_SIZE_USER64 - 1)
>> #else
>> #define USER_DS         MAKE_MM_SEG(TASK_SIZE - 1)
>> #endif
>
> Where do you write a smaller value than USER_DS into the addr_limit?

I don't think we do.

> The kernel is full of places that assume that any access up to USER_DS
> is safe; for example, perf_output_sample_ustack(),
> get_perf_callchain(), do_exit(), flush_old_exec(), vma_dump_size(),
> ... - and I also don't see anything in the powerpc code that would
> ever write a smaller value into the addr_limit.
>
> I don't know powerpc well, but AFAIK the rule on X86 is basically that
> even for compat tasks, attempting to access anything up to USER_DS is
> safe because the kernel doesn't put any kernel mappings there. Is that
> different on powerpc?

That's how it works on powerpc too.

A compat task will have a TASK_SIZE of 4GB - 1 page, but the kernel is
still at c000000000000000 and there aren't any kernel mappings below
that. So it's still safe to just check against the maximum possible user
address, which is 4PB.

cheers

Re: [PATCH] powerpc: Don't print kernel instructions in show_user_instructions()

[Michael Ellerman]

Christophe LEROY <christophe.leroy@c-s.fr> writes:
> Le 05/10/2018 à 15:21, Michael Ellerman a écrit :
>> Recently we implemented show_user_instructions() which dumps the code
>> around the NIP when a user space process dies with an unhandled
>> signal. This was modelled on the x86 code, and we even went so far as
>> to implement the exact same bug, namely that if the user process
>> crashed with its NIP pointing into the kernel we will dump kernel text
>> to dmesg. eg:
>> 
>>    bad-bctr[2996]: segfault (11) at c000000000010000 nip c000000000010000 lr 12d0b0894 code 1
>>    bad-bctr[2996]: code: fbe10068 7cbe2b78 7c7f1b78 fb610048 38a10028 38810020 fb810050 7f8802a6
>>    bad-bctr[2996]: code: 3860001c f8010080 48242371 60000000 <7c7b1b79> 4082002c e8010080 eb610048
>> 
>> This was discovered on x86 by Jann Horn and fixed in commit
>> 342db04ae712 ("x86/dumpstack: Don't dump kernel memory based on usermode RIP").
>> 
>> Fix it by checking the adjusted NIP value (pc) and number of
>> instructions against USER_DS, and bail if we fail the check, eg:
>> 
>>    bad-bctr[2969]: segfault (11) at c000000000010000 nip c000000000010000 lr 107930894 code 1
>>    bad-bctr[2969]: Bad NIP, not dumping instructions.
>> 
>> Fixes: 88b0fe175735 ("powerpc: Add show_user_instructions()")
>> Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
>> ---
>>   arch/powerpc/kernel/process.c | 10 ++++++++++
>>   1 file changed, 10 insertions(+)
>> 
>> diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.c
>> index 913c5725cdb2..bb6ac471a784 100644
>> --- a/arch/powerpc/kernel/process.c
>> +++ b/arch/powerpc/kernel/process.c
>> @@ -1306,6 +1306,16 @@ void show_user_instructions(struct pt_regs *regs)
>>   
>>   	pc = regs->nip - (instructions_to_print * 3 / 4 * sizeof(int));
>>   
>> +	/*
>> +	 * Make sure the NIP points at userspace, not kernel text/data or
>> +	 * elsewhere.
>> +	 */
>> +	if (!__access_ok(pc, instructions_to_print * sizeof(int), USER_DS)) {
>> +		pr_info("%s[%d]: Bad NIP, not dumping instructions.\n",
>> +			current->comm, current->pid);
>> +		return;
>> +	}
>> +
>
> Is there any reason for not using access_ok() here ?

I wanted to check against USER_DS explicitly. But maybe that was
over-paranoid of me.

cheers