parisc: Fix kernel panic due invalid values of IAOQ0 or IAOQ1

Commit Message

Helge Deller July 3, 2019, 6:35 a.m. UTC

On parisc the privilege level of a process is stored in the lowest two bits of
the instruction pointers (IAOQ0 and IAOQ1). On Linux we use privilege level 0
for the kernel and privilege level 3 for user-space. So userspace should not be
allowed to modify IAOQ0 or IAOQ1 of a ptraced process to change it's privilege
level to e.g. 0 to try to gain kernel privileges.

This patch prevents such modifications by always setting the two lowest bits to
one (which relates to privilege level 3 for user-space) if IAOQ0 or IAOQ1 are
modified via ptrace calls.

Fixes: https://bugs.gentoo.org/481768
Reported-by: Jeroen Roovers <[e-mail]>
Cc: <[e-mail]>

Comments

Jeroen Roovers July 4, 2019, 7:23 p.m. UTC | #1

On Wed, 3 Jul 2019 08:35:24 +0200
Helge Deller <[e-mail]> wrote:

> On parisc the privilege level of a process is stored in the lowest
> two bits of the instruction pointers (IAOQ0 and IAOQ1). On Linux we
> use privilege level 0 for the kernel and privilege level 3 for
> user-space. So userspace should not be allowed to modify IAOQ0 or
> IAOQ1 of a ptraced process to change it's privilege level to e.g. 0
> to try to gain kernel privileges.
> 
> This patch prevents such modifications by always setting the two
> lowest bits to one (which relates to privilege level 3 for
> user-space) if IAOQ0 or IAOQ1 are modified via ptrace calls.
> 
> Fixes: https://bugs.gentoo.org/481768
> Reported-by: Jeroen Roovers <[e-mail]>
> Cc: <[e-mail]>
> 
> diff --git a/arch/parisc/kernel/ptrace.c b/arch/parisc/kernel/ptrace.c
> index a3d2fb4e6dd2..8ecd41938709 100644
> --- a/arch/parisc/kernel/ptrace.c
> +++ b/arch/parisc/kernel/ptrace.c
> @@ -167,6 +175,9 @@ long arch_ptrace(struct task_struct *child, long
> request, if ((addr & (sizeof(unsigned long)-1)) ||
>  		     addr >= sizeof(struct pt_regs))
>  			break;
> +		if (addr == PT_IAOQ0 || addr == PT_IAOQ1) {
> +			data |= 3; /* ensure userspace privilege */
> +		}
>  		if ((addr >= PT_GR1 && addr <= PT_GR31) ||
>  				addr == PT_IAOQ0 || addr == PT_IAOQ1
> || (addr >= PT_FR0 && addr <= PT_FR31 + 4) ||
> @@ -281,6 +292,9 @@ long compat_arch_ptrace(struct task_struct
> *child, compat_long_t request, addr = translate_usr_offset(addr);
>  			if (addr >= sizeof(struct pt_regs))
>  				break;
> +			if (addr == PT_IAOQ0 || addr == PT_IAOQ1) {
> +				data |= 3; /* ensure userspace
> privilege */
> +			}
>  			if (addr >= PT_FR0 && addr <= PT_FR31 + 4) {
>  				/* Special case, fp regs are 64 bits
> anyway */ *(__u64 *) ((char *) task_regs(child) + addr) = data;

That may fix some problem, but it sadly does not fix the problem
reported in https://bugs.gentoo.org/481768 . Both root and unprivileged
users can still trigger the same kernel panic with a kernel patches
thusly. How can we help you reproduce the issue?


Kind regards,
      jer

Helge Deller July 4, 2019, 7:54 p.m. UTC | #2

Hi Jeroen,

On 04.07.19 21:23, Jeroen Roovers wrote:
> On Wed, 3 Jul 2019 08:35:24 +0200
> Helge Deller <[e-mail]> wrote:
>
>> On parisc the privilege level of a process is stored in the lowest
>> two bits of the instruction pointers (IAOQ0 and IAOQ1). On Linux we
>> use privilege level 0 for the kernel and privilege level 3 for
>> user-space. So userspace should not be allowed to modify IAOQ0 or
>> IAOQ1 of a ptraced process to change it's privilege level to e.g. 0
>> to try to gain kernel privileges.
>>
>> This patch prevents such modifications by always setting the two
>> lowest bits to one (which relates to privilege level 3 for
>> user-space) if IAOQ0 or IAOQ1 are modified via ptrace calls.
>>
>> Fixes: https://bugs.gentoo.org/481768
>> Reported-by: Jeroen Roovers <[e-mail]>
>> Cc: <[e-mail]>
>>
>> diff --git a/arch/parisc/kernel/ptrace.c b/arch/parisc/kernel/ptrace.c
>> index a3d2fb4e6dd2..8ecd41938709 100644
>> --- a/arch/parisc/kernel/ptrace.c
>> +++ b/arch/parisc/kernel/ptrace.c
>> @@ -167,6 +175,9 @@ long arch_ptrace(struct task_struct *child, long
>> request, if ((addr & (sizeof(unsigned long)-1)) ||
>>   		     addr >= sizeof(struct pt_regs))
>>   			break;
>> +		if (addr == PT_IAOQ0 || addr == PT_IAOQ1) {
>> +			data |= 3; /* ensure userspace privilege */
>> +		}
>>   		if ((addr >= PT_GR1 && addr <= PT_GR31) ||
>>   				addr == PT_IAOQ0 || addr == PT_IAOQ1
>> || (addr >= PT_FR0 && addr <= PT_FR31 + 4) ||
>> @@ -281,6 +292,9 @@ long compat_arch_ptrace(struct task_struct
>> *child, compat_long_t request, addr = translate_usr_offset(addr);
>>   			if (addr >= sizeof(struct pt_regs))
>>   				break;
>> +			if (addr == PT_IAOQ0 || addr == PT_IAOQ1) {
>> +				data |= 3; /* ensure userspace
>> privilege */
>> +			}
>>   			if (addr >= PT_FR0 && addr <= PT_FR31 + 4) {
>>   				/* Special case, fp regs are 64 bits
>> anyway */ *(__u64 *) ((char *) task_regs(child) + addr) = data;
>
> That may fix some problem, but it sadly does not fix the problem
> reported in https://bugs.gentoo.org/481768 . Both root and unprivileged
> users can still trigger the same kernel panic with a kernel patches
> thusly. How can we help you reproduce the issue?

Thanks for testing, but are you sure?
It does fix exactly that kernel panic for me.
Instead of triggering a kernel panic, the ptraced process now gets
killed by a segfault, which is exactly what should happen:

Breakpoint 1, main () at gdb-crash.c:24
24      gdb-crash.c: No such file or directory.
(gdb) set tp = { 3,4 }
Python Exception <type 'exceptions.NameError'> Installation error: gdb.execute_unwinders function is missing:
Program received signal SIGSEGV, Segmentation fault.
0x00000000 in ?? ()
The program being debugged was signaled while in a function called from GDB.
GDB remains in the frame where the signal was received.
To change this behavior use "set unwindonsignal on".
Evaluation of the expression containing the function
(malloc) will be abandoned.
When the function is done executing, GDB will silently stop.
(gdb)

Of course this patch does not fix gdb in such a way that
it handles the "set tp = { 5,3 }" correctly. That's a gdb issue.

The above log is from a 32bit-kernel. Did you maybe tested a 64bit kernel (I didn't tested it).
Or maybe you didn't booted a kernel with that patch?
I'm pretty sure the patch is correct.

Helge

Helge Deller July 4, 2019, 7:58 p.m. UTC | #3

On 04.07.19 21:54, Helge Deller wrote:
> Hi Jeroen,
>
> On 04.07.19 21:23, Jeroen Roovers wrote:
>> On Wed, 3 Jul 2019 08:35:24 +0200
>> Helge Deller <[e-mail]> wrote:
>>
>>> On parisc the privilege level of a process is stored in the lowest
>>> two bits of the instruction pointers (IAOQ0 and IAOQ1). On Linux we
>>> use privilege level 0 for the kernel and privilege level 3 for
>>> user-space. So userspace should not be allowed to modify IAOQ0 or
>>> IAOQ1 of a ptraced process to change it's privilege level to e.g. 0
>>> to try to gain kernel privileges.
>>>
>>> This patch prevents such modifications by always setting the two
>>> lowest bits to one (which relates to privilege level 3 for
>>> user-space) if IAOQ0 or IAOQ1 are modified via ptrace calls.
>>>
>>> Fixes: https://bugs.gentoo.org/481768
>>> Reported-by: Jeroen Roovers <[e-mail]>
>>> Cc: <[e-mail]>
>>>
>>> diff --git a/arch/parisc/kernel/ptrace.c b/arch/parisc/kernel/ptrace.c
>>> index a3d2fb4e6dd2..8ecd41938709 100644
>>> --- a/arch/parisc/kernel/ptrace.c
>>> +++ b/arch/parisc/kernel/ptrace.c
>>> @@ -167,6 +175,9 @@ long arch_ptrace(struct task_struct *child, long
>>> request, if ((addr & (sizeof(unsigned long)-1)) ||
>>>                addr >= sizeof(struct pt_regs))
>>>               break;
>>> +        if (addr == PT_IAOQ0 || addr == PT_IAOQ1) {
>>> +            data |= 3; /* ensure userspace privilege */
>>> +        }
>>>           if ((addr >= PT_GR1 && addr <= PT_GR31) ||
>>>                   addr == PT_IAOQ0 || addr == PT_IAOQ1
>>> || (addr >= PT_FR0 && addr <= PT_FR31 + 4) ||
>>> @@ -281,6 +292,9 @@ long compat_arch_ptrace(struct task_struct
>>> *child, compat_long_t request, addr = translate_usr_offset(addr);
>>>               if (addr >= sizeof(struct pt_regs))
>>>                   break;
>>> +            if (addr == PT_IAOQ0 || addr == PT_IAOQ1) {
>>> +                data |= 3; /* ensure userspace
>>> privilege */
>>> +            }
>>>               if (addr >= PT_FR0 && addr <= PT_FR31 + 4) {
>>>                   /* Special case, fp regs are 64 bits
>>> anyway */ *(__u64 *) ((char *) task_regs(child) + addr) = data;
>>
>> That may fix some problem, but it sadly does not fix the problem
>> reported in https://bugs.gentoo.org/481768 . Both root and unprivileged
>> users can still trigger the same kernel panic with a kernel patches
>> thusly. How can we help you reproduce the issue?
>
> Thanks for testing, but are you sure?
> It does fix exactly that kernel panic for me.
> Instead of triggering a kernel panic, the ptraced process now gets
> killed by a segfault, which is exactly what should happen:
>
> Breakpoint 1, main () at gdb-crash.c:24
> 24      gdb-crash.c: No such file or directory.
> (gdb) set tp = { 3,4 }
> Python Exception <type 'exceptions.NameError'> Installation error: gdb.execute_unwinders function is missing:
> Program received signal SIGSEGV, Segmentation fault.
> 0x00000000 in ?? ()
> The program being debugged was signaled while in a function called from GDB.
> GDB remains in the frame where the signal was received.
> To change this behavior use "set unwindonsignal on".
> Evaluation of the expression containing the function
> (malloc) will be abandoned.
> When the function is done executing, GDB will silently stop.
> (gdb)
>
> Of course this patch does not fix gdb in such a way that
> it handles the "set tp = { 5,3 }" correctly. That's a gdb issue.
>
> The above log is from a 32bit-kernel. Did you maybe tested a 64bit kernel (I didn't tested it).
> Or maybe you didn't booted a kernel with that patch?
> I'm pretty sure the patch is correct.

In case you still get a kernel panic, please verify the value of "IAOQ" in the panic you see:
IASQ: 0000000000334000 0000000000334000 IAOQ: 0000000000000000 0000000000000004

If the value is still "0000000000000000" instead of "0000000000000003",
then my patch wasn't applied correctly.

Helge

Helge Deller July 4, 2019, 8:36 p.m. UTC | #4

On 04.07.19 21:58, Helge Deller wrote:
> On 04.07.19 21:54, Helge Deller wrote:
>> Hi Jeroen,
>>
>> On 04.07.19 21:23, Jeroen Roovers wrote:
>>> On Wed, 3 Jul 2019 08:35:24 +0200
>>> Helge Deller <[e-mail]> wrote:
>>>
>>>> On parisc the privilege level of a process is stored in the lowest
>>>> two bits of the instruction pointers (IAOQ0 and IAOQ1). On Linux we
>>>> use privilege level 0 for the kernel and privilege level 3 for
>>>> user-space. So userspace should not be allowed to modify IAOQ0 or
>>>> IAOQ1 of a ptraced process to change it's privilege level to e.g. 0
>>>> to try to gain kernel privileges.
>>>>
>>>> This patch prevents such modifications by always setting the two
>>>> lowest bits to one (which relates to privilege level 3 for
>>>> user-space) if IAOQ0 or IAOQ1 are modified via ptrace calls.
>>>>
>>>> Fixes: https://bugs.gentoo.org/481768
>>>> Reported-by: Jeroen Roovers <[e-mail]>
>>>> Cc: <[e-mail]>
>>>>
>>>> diff --git a/arch/parisc/kernel/ptrace.c b/arch/parisc/kernel/ptrace.c
>>>> index a3d2fb4e6dd2..8ecd41938709 100644
>>>> --- a/arch/parisc/kernel/ptrace.c
>>>> +++ b/arch/parisc/kernel/ptrace.c
>>>> @@ -167,6 +175,9 @@ long arch_ptrace(struct task_struct *child, long
>>>> request, if ((addr & (sizeof(unsigned long)-1)) ||
>>>>                addr >= sizeof(struct pt_regs))
>>>>               break;
>>>> +        if (addr == PT_IAOQ0 || addr == PT_IAOQ1) {
>>>> +            data |= 3; /* ensure userspace privilege */
>>>> +        }
>>>>           if ((addr >= PT_GR1 && addr <= PT_GR31) ||
>>>>                   addr == PT_IAOQ0 || addr == PT_IAOQ1
>>>> || (addr >= PT_FR0 && addr <= PT_FR31 + 4) ||
>>>> @@ -281,6 +292,9 @@ long compat_arch_ptrace(struct task_struct
>>>> *child, compat_long_t request, addr = translate_usr_offset(addr);
>>>>               if (addr >= sizeof(struct pt_regs))
>>>>                   break;
>>>> +            if (addr == PT_IAOQ0 || addr == PT_IAOQ1) {
>>>> +                data |= 3; /* ensure userspace
>>>> privilege */
>>>> +            }
>>>>               if (addr >= PT_FR0 && addr <= PT_FR31 + 4) {
>>>>                   /* Special case, fp regs are 64 bits
>>>> anyway */ *(__u64 *) ((char *) task_regs(child) + addr) = data;
>>>
>>> That may fix some problem, but it sadly does not fix the problem
>>> reported in https://bugs.gentoo.org/481768 . Both root and unprivileged
>>> users can still trigger the same kernel panic with a kernel patches
>>> thusly. How can we help you reproduce the issue?
>>
>> Thanks for testing, but are you sure?
>> It does fix exactly that kernel panic for me.
>> Instead of triggering a kernel panic, the ptraced process now gets
>> killed by a segfault, which is exactly what should happen:
>>
>> Breakpoint 1, main () at gdb-crash.c:24
>> 24      gdb-crash.c: No such file or directory.
>> (gdb) set tp = { 3,4 }
>> Python Exception <type 'exceptions.NameError'> Installation error: gdb.execute_unwinders function is missing:
>> Program received signal SIGSEGV, Segmentation fault.
>> 0x00000000 in ?? ()
>> The program being debugged was signaled while in a function called from GDB.
>> GDB remains in the frame where the signal was received.
>> To change this behavior use "set unwindonsignal on".
>> Evaluation of the expression containing the function
>> (malloc) will be abandoned.
>> When the function is done executing, GDB will silently stop.
>> (gdb)
>>
>> Of course this patch does not fix gdb in such a way that
>> it handles the "set tp = { 5,3 }" correctly. That's a gdb issue.
>>
>> The above log is from a 32bit-kernel. Did you maybe tested a 64bit kernel (I didn't tested it).
>> Or maybe you didn't booted a kernel with that patch?
>> I'm pretty sure the patch is correct.
>
> In case you still get a kernel panic, please verify the value of "IAOQ" in the panic you see:
> IASQ: 0000000000334000 0000000000334000 IAOQ: 0000000000000000 0000000000000004
>
> If the value is still "0000000000000000" instead of "0000000000000003",
> then my patch wasn't applied correctly.

I'm wrong.
I see crashes with a 64bit kernel too. 32bit kernel worked nicely.
Will analyze.

Helge

Patch

diff --git a/arch/parisc/kernel/ptrace.c b/arch/parisc/kernel/ptrace.c
index a3d2fb4e6dd2..8ecd41938709 100644
--- a/arch/parisc/kernel/ptrace.c
+++ b/arch/parisc/kernel/ptrace.c
@@ -167,6 +175,9 @@  long arch_ptrace(struct task_struct *child, long request,
 		if ((addr & (sizeof(unsigned long)-1)) ||
 		     addr >= sizeof(struct pt_regs))
 			break;
+		if (addr == PT_IAOQ0 || addr == PT_IAOQ1) {
+			data |= 3; /* ensure userspace privilege */
+		}
 		if ((addr >= PT_GR1 && addr <= PT_GR31) ||
 				addr == PT_IAOQ0 || addr == PT_IAOQ1 ||
 				(addr >= PT_FR0 && addr <= PT_FR31 + 4) ||
@@ -281,6 +292,9 @@  long compat_arch_ptrace(struct task_struct *child, compat_long_t request,
 			addr = translate_usr_offset(addr);
 			if (addr >= sizeof(struct pt_regs))
 				break;
+			if (addr == PT_IAOQ0 || addr == PT_IAOQ1) {
+				data |= 3; /* ensure userspace privilege */
+			}
 			if (addr >= PT_FR0 && addr <= PT_FR31 + 4) {
 				/* Special case, fp regs are 64 bits anyway */
 				*(__u64 *) ((char *) task_regs(child) + addr) = data;