* [PATCH] powerpc/ptrace: Mitigate potential Spectre v1
@ 2019-01-24 14:01 Breno Leitao
2019-01-24 17:25 ` Gustavo A. R. Silva
0 siblings, 1 reply; 4+ messages in thread
From: Breno Leitao @ 2019-01-24 14:01 UTC (permalink / raw)
To: linuxppc-dev; +Cc: Breno Leitao, gustavo
'regno' is directly controlled by user space, hence leading to a potential
exploitation of the Spectre variant 1 vulnerability.
On PTRACE_SETREGS and PTRACE_GETREGS requests, user space passes the
register number that would be read or written. This register number is
called 'regno' which is part of the 'addr' syscall parameter.
This 'regno' value is checked against the maximum pt_regs structure size,
and then used to dereference it, which matches the initial part of a
Spectre v1 (and Spectre v1.1) attack. The dereferenced value, then,
is returned to userspace in the GETREGS case.
This patch sanitizes 'regno' before using it to dereference pt_reg.
Notice that given that speculation windows are large, the policy is
to kill the speculation on the first load and not worry if it can be
completed with a dependent load/store [1].
[1] https://marc.info/?l=linux-kernel&m=152449131114778&w=2
Signed-off-by: Breno Leitao <leitao@debian.org>
---
arch/powerpc/kernel/ptrace.c | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/arch/powerpc/kernel/ptrace.c b/arch/powerpc/kernel/ptrace.c
index cdd5d1d3ae41..3eac38a29863 100644
--- a/arch/powerpc/kernel/ptrace.c
+++ b/arch/powerpc/kernel/ptrace.c
@@ -33,6 +33,7 @@
#include <linux/hw_breakpoint.h>
#include <linux/perf_event.h>
#include <linux/context_tracking.h>
+#include <linux/nospec.h>
#include <linux/uaccess.h>
#include <linux/pkeys.h>
@@ -298,6 +299,9 @@ int ptrace_get_reg(struct task_struct *task, int regno, unsigned long *data)
#endif
if (regno < (sizeof(struct user_pt_regs) / sizeof(unsigned long))) {
+ regno = array_index_nospec(regno,
+ (sizeof(struct user_pt_regs) /
+ sizeof(unsigned long)));
*data = ((unsigned long *)task->thread.regs)[regno];
return 0;
}
@@ -321,6 +325,7 @@ int ptrace_put_reg(struct task_struct *task, int regno, unsigned long data)
return set_user_dscr(task, data);
if (regno <= PT_MAX_PUT_REG) {
+ regno = array_index_nospec(regno, PT_MAX_PUT_REG);
((unsigned long *)task->thread.regs)[regno] = data;
return 0;
}
--
2.19.0
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] powerpc/ptrace: Mitigate potential Spectre v1
2019-01-24 14:01 [PATCH] powerpc/ptrace: Mitigate potential Spectre v1 Breno Leitao
@ 2019-01-24 17:25 ` Gustavo A. R. Silva
2019-01-29 16:38 ` Breno Leitao
0 siblings, 1 reply; 4+ messages in thread
From: Gustavo A. R. Silva @ 2019-01-24 17:25 UTC (permalink / raw)
To: Breno Leitao, linuxppc-dev
On 1/24/19 8:01 AM, Breno Leitao wrote:
> 'regno' is directly controlled by user space, hence leading to a potential
> exploitation of the Spectre variant 1 vulnerability.
>
> On PTRACE_SETREGS and PTRACE_GETREGS requests, user space passes the
> register number that would be read or written. This register number is
> called 'regno' which is part of the 'addr' syscall parameter.
>
> This 'regno' value is checked against the maximum pt_regs structure size,
> and then used to dereference it, which matches the initial part of a
> Spectre v1 (and Spectre v1.1) attack. The dereferenced value, then,
> is returned to userspace in the GETREGS case.
>
Was this reported by any tool?
If so, it might be worth mentioning it.
> This patch sanitizes 'regno' before using it to dereference pt_reg.
>
> Notice that given that speculation windows are large, the policy is
> to kill the speculation on the first load and not worry if it can be
> completed with a dependent load/store [1].
>
> [1] https://marc.info/?l=linux-kernel&m=152449131114778&w=2
>
> Signed-off-by: Breno Leitao <leitao@debian.org>
> ---
> arch/powerpc/kernel/ptrace.c | 5 +++++
> 1 file changed, 5 insertions(+)
>
> diff --git a/arch/powerpc/kernel/ptrace.c b/arch/powerpc/kernel/ptrace.c
> index cdd5d1d3ae41..3eac38a29863 100644
> --- a/arch/powerpc/kernel/ptrace.c
> +++ b/arch/powerpc/kernel/ptrace.c
> @@ -33,6 +33,7 @@
> #include <linux/hw_breakpoint.h>
> #include <linux/perf_event.h>
> #include <linux/context_tracking.h>
> +#include <linux/nospec.h>
>
> #include <linux/uaccess.h>
> #include <linux/pkeys.h>
> @@ -298,6 +299,9 @@ int ptrace_get_reg(struct task_struct *task, int regno, unsigned long *data)
> #endif
>
> if (regno < (sizeof(struct user_pt_regs) / sizeof(unsigned long))) {
I would use a variable to store sizeof(struct user_pt_regs) / sizeof(unsigned long).
> + regno = array_index_nospec(regno,
> + (sizeof(struct user_pt_regs) /
> + sizeof(unsigned long)));
See the rest of my comments below.
> *data = ((unsigned long *)task->thread.regs)[regno];
> return 0;
> }
> @@ -321,6 +325,7 @@ int ptrace_put_reg(struct task_struct *task, int regno, unsigned long data)
> return set_user_dscr(task, data);
>
> if (regno <= PT_MAX_PUT_REG) {
> + regno = array_index_nospec(regno, PT_MAX_PUT_REG);
This is wrong. array_index_nospec() will return PT_MAX_PUT_REG - 1 in case regno is equal to
PT_MAX_PUT_REG, and this is not what you want.
Similar reasoning applies to the case above.
> ((unsigned long *)task->thread.regs)[regno] = data;
> return 0;
> }
>
Thanks
--
Gustavo
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] powerpc/ptrace: Mitigate potential Spectre v1
2019-01-24 17:25 ` Gustavo A. R. Silva
@ 2019-01-29 16:38 ` Breno Leitao
2019-01-29 17:00 ` Gustavo A. R. Silva
0 siblings, 1 reply; 4+ messages in thread
From: Breno Leitao @ 2019-01-29 16:38 UTC (permalink / raw)
To: Gustavo A. R. Silva, linuxppc-dev
Hi Gustavo,
On 1/24/19 3:25 PM, Gustavo A. R. Silva wrote:
>
>
> On 1/24/19 8:01 AM, Breno Leitao wrote:
>> 'regno' is directly controlled by user space, hence leading to a potential
>> exploitation of the Spectre variant 1 vulnerability.
>>
>> On PTRACE_SETREGS and PTRACE_GETREGS requests, user space passes the
>> register number that would be read or written. This register number is
>> called 'regno' which is part of the 'addr' syscall parameter.
>>
>> This 'regno' value is checked against the maximum pt_regs structure size,
>> and then used to dereference it, which matches the initial part of a
>> Spectre v1 (and Spectre v1.1) attack. The dereferenced value, then,
>> is returned to userspace in the GETREGS case.
>>
>
> Was this reported by any tool?
It was not. I was writing an userspace tool, which required me to read the
Ptrace subsystem carefully, then, I just found this case.
> If so, it might be worth mentioning it.
>
>> This patch sanitizes 'regno' before using it to dereference pt_reg.
>>
>> Notice that given that speculation windows are large, the policy is
>> to kill the speculation on the first load and not worry if it can be
>> completed with a dependent load/store [1].
>>
>> [1] https://marc.info/?l=linux-kernel&m=152449131114778&w=2
>>
>> Signed-off-by: Breno Leitao <leitao@debian.org>
>> ---
>> arch/powerpc/kernel/ptrace.c | 5 +++++
>> 1 file changed, 5 insertions(+)
>>
>> diff --git a/arch/powerpc/kernel/ptrace.c b/arch/powerpc/kernel/ptrace.c
>> index cdd5d1d3ae41..3eac38a29863 100644
>> --- a/arch/powerpc/kernel/ptrace.c
>> +++ b/arch/powerpc/kernel/ptrace.c
>> @@ -33,6 +33,7 @@
>> #include <linux/hw_breakpoint.h>
>> #include <linux/perf_event.h>
>> #include <linux/context_tracking.h>
>> +#include <linux/nospec.h>
>>
>> #include <linux/uaccess.h>
>> #include <linux/pkeys.h>
>> @@ -298,6 +299,9 @@ int ptrace_get_reg(struct task_struct *task, int regno, unsigned long *data)
>> #endif
>>
>> if (regno < (sizeof(struct user_pt_regs) / sizeof(unsigned long))) {
>
> I would use a variable to store sizeof(struct user_pt_regs) / sizeof(unsigned long).
Right.
>
>> + regno = array_index_nospec(regno,
>> + (sizeof(struct user_pt_regs) /
>> + sizeof(unsigned long)));
>
> See the rest of my comments below.
>
>> *data = ((unsigned long *)task->thread.regs)[regno];
>> return 0;
>> }
>> @@ -321,6 +325,7 @@ int ptrace_put_reg(struct task_struct *task, int regno, unsigned long data)
>> return set_user_dscr(task, data);
>>
>> if (regno <= PT_MAX_PUT_REG) {
>> + regno = array_index_nospec(regno, PT_MAX_PUT_REG);
>
> This is wrong. array_index_nospec() will return PT_MAX_PUT_REG - 1 in case regno is equal to
> PT_MAX_PUT_REG, and this is not what you want.
Right, this is really wrong. It would be correct if the comparison was
regno < PT_MAX_PUT_REG. Since it is PT_MAX_PUT_REGS is a valid array entry,
then I need to care about this case also. Doing something as:
regno = array_index_nospec(regno, PT_MAX_PUT_REG + 1);
Other than that, I think that for the regno = PT_MAX_PUT_REG base,
array_index_nospec() will redefine regno to 0, not PT_MAX_PUT_REG - 1.
> Similar reasoning applies to the case above.
I understand that the case above does not seem to have the same problem,
since it does not address over the array as done in the case above. Does it
make sense?
Anyway, this is how I am thinking the v2 of the patch:
diff --git a/arch/powerpc/kernel/ptrace.c b/arch/powerpc/kernel/ptrace.c
index cdd5d1d3ae41..7535f89e08cd 100644
--- a/arch/powerpc/kernel/ptrace.c
+++ b/arch/powerpc/kernel/ptrace.c
@@ -33,6 +33,7 @@
#include <linux/hw_breakpoint.h>
#include <linux/perf_event.h>
#include <linux/context_tracking.h>
+#include <linux/nospec.h>
#include <linux/uaccess.h>
#include <linux/pkeys.h>
@@ -274,6 +275,8 @@ static int set_user_trap(struct task_struct *task,
unsigned long trap)
*/
int ptrace_get_reg(struct task_struct *task, int regno, unsigned long *data)
{
+ unsigned int regs_max;
+
if ((task->thread.regs == NULL) || !data)
return -EIO;
@@ -297,7 +300,9 @@ int ptrace_get_reg(struct task_struct *task, int regno,
unsigned long *data)
}
#endif
- if (regno < (sizeof(struct user_pt_regs) / sizeof(unsigned long))) {
+ regs_max = sizeof(struct user_pt_regs) / sizeof(unsigned long);
+ if (regno < regs_max) {
+ regno = array_index_nospec(regno, regs_max);
*data = ((unsigned long *)task->thread.regs)[regno];
return 0;
}
@@ -321,6 +326,7 @@ int ptrace_put_reg(struct task_struct *task, int regno,
unsigned long data)
return set_user_dscr(task, data);
if (regno <= PT_MAX_PUT_REG) {
+ regno = array_index_nospec(regno, PT_MAX_PUT_REG + 1);
((unsigned long *)task->thread.regs)[regno] = data;
return 0;
}
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] powerpc/ptrace: Mitigate potential Spectre v1
2019-01-29 16:38 ` Breno Leitao
@ 2019-01-29 17:00 ` Gustavo A. R. Silva
0 siblings, 0 replies; 4+ messages in thread
From: Gustavo A. R. Silva @ 2019-01-29 17:00 UTC (permalink / raw)
To: Breno Leitao, linuxppc-dev
Hi Breno,
On 1/29/19 10:38 AM, Breno Leitao wrote:
> Hi Gustavo,
>
> On 1/24/19 3:25 PM, Gustavo A. R. Silva wrote:
>>
>>
>> On 1/24/19 8:01 AM, Breno Leitao wrote:
>>> 'regno' is directly controlled by user space, hence leading to a potential
>>> exploitation of the Spectre variant 1 vulnerability.
>>>
>>> On PTRACE_SETREGS and PTRACE_GETREGS requests, user space passes the
>>> register number that would be read or written. This register number is
>>> called 'regno' which is part of the 'addr' syscall parameter.
>>>
>>> This 'regno' value is checked against the maximum pt_regs structure size,
>>> and then used to dereference it, which matches the initial part of a
>>> Spectre v1 (and Spectre v1.1) attack. The dereferenced value, then,
>>> is returned to userspace in the GETREGS case.
>>>
>>
>> Was this reported by any tool?
>
> It was not. I was writing an userspace tool, which required me to read the
> Ptrace subsystem carefully, then, I just found this case.
>
Great. Good catch.
>> If so, it might be worth mentioning it.
>>
>>> This patch sanitizes 'regno' before using it to dereference pt_reg.
>>>
>>> Notice that given that speculation windows are large, the policy is
>>> to kill the speculation on the first load and not worry if it can be
>>> completed with a dependent load/store [1].
>>>
>>> [1] https://marc.info/?l=linux-kernel&m=152449131114778&w=2
>>>
>>> Signed-off-by: Breno Leitao <leitao@debian.org>
>>> ---
>>> arch/powerpc/kernel/ptrace.c | 5 +++++
>>> 1 file changed, 5 insertions(+)
>>>
>>> diff --git a/arch/powerpc/kernel/ptrace.c b/arch/powerpc/kernel/ptrace.c
>>> index cdd5d1d3ae41..3eac38a29863 100644
>>> --- a/arch/powerpc/kernel/ptrace.c
>>> +++ b/arch/powerpc/kernel/ptrace.c
>>> @@ -33,6 +33,7 @@
>>> #include <linux/hw_breakpoint.h>
>>> #include <linux/perf_event.h>
>>> #include <linux/context_tracking.h>
>>> +#include <linux/nospec.h>
>>>
>>> #include <linux/uaccess.h>
>>> #include <linux/pkeys.h>
>>> @@ -298,6 +299,9 @@ int ptrace_get_reg(struct task_struct *task, int regno, unsigned long *data)
>>> #endif
>>>
>>> if (regno < (sizeof(struct user_pt_regs) / sizeof(unsigned long))) {
>>
>> I would use a variable to store sizeof(struct user_pt_regs) / sizeof(unsigned long).
>
> Right.
>
>>
>>> + regno = array_index_nospec(regno,
>>> + (sizeof(struct user_pt_regs) /
>>> + sizeof(unsigned long)));
>>
>> See the rest of my comments below.
>>
>>> *data = ((unsigned long *)task->thread.regs)[regno];
>>> return 0;
>>> }
>>> @@ -321,6 +325,7 @@ int ptrace_put_reg(struct task_struct *task, int regno, unsigned long data)
>>> return set_user_dscr(task, data);
>>>
>>> if (regno <= PT_MAX_PUT_REG) {
>>> + regno = array_index_nospec(regno, PT_MAX_PUT_REG);
>>
>> This is wrong. array_index_nospec() will return PT_MAX_PUT_REG - 1 in case regno is equal to
>> PT_MAX_PUT_REG, and this is not what you want.
>
> Right, this is really wrong. It would be correct if the comparison was
> regno < PT_MAX_PUT_REG. Since it is PT_MAX_PUT_REGS is a valid array entry,
> then I need to care about this case also. Doing something as:
>
> regno = array_index_nospec(regno, PT_MAX_PUT_REG + 1);
>
This is the right way.
> Other than that, I think that for the regno = PT_MAX_PUT_REG base,
> array_index_nospec() will redefine regno to 0, not PT_MAX_PUT_REG - 1.
>
>> Similar reasoning applies to the case above.
>
> I understand that the case above does not seem to have the same problem,
> since it does not address over the array as done in the case above. Does it
> make sense?
>
Yeah. I was wrong about that. If regno is out of bounds, array_index_nospec()
will return 0.
> Anyway, this is how I am thinking the v2 of the patch:
>
V2 looks good.
Thanks
--
Gustavo
> diff --git a/arch/powerpc/kernel/ptrace.c b/arch/powerpc/kernel/ptrace.c
> index cdd5d1d3ae41..7535f89e08cd 100644
> --- a/arch/powerpc/kernel/ptrace.c
> +++ b/arch/powerpc/kernel/ptrace.c
> @@ -33,6 +33,7 @@
> #include <linux/hw_breakpoint.h>
> #include <linux/perf_event.h>
> #include <linux/context_tracking.h>
> +#include <linux/nospec.h>
>
> #include <linux/uaccess.h>
> #include <linux/pkeys.h>
> @@ -274,6 +275,8 @@ static int set_user_trap(struct task_struct *task,
> unsigned long trap)
> */
> int ptrace_get_reg(struct task_struct *task, int regno, unsigned long *data)
> {
> + unsigned int regs_max;
> +
> if ((task->thread.regs == NULL) || !data)
> return -EIO;
>
> @@ -297,7 +300,9 @@ int ptrace_get_reg(struct task_struct *task, int regno,
> unsigned long *data)
> }
> #endif
>
> - if (regno < (sizeof(struct user_pt_regs) / sizeof(unsigned long))) {
> + regs_max = sizeof(struct user_pt_regs) / sizeof(unsigned long);
> + if (regno < regs_max) {
> + regno = array_index_nospec(regno, regs_max);
> *data = ((unsigned long *)task->thread.regs)[regno];
> return 0;
> }
> @@ -321,6 +326,7 @@ int ptrace_put_reg(struct task_struct *task, int regno,
> unsigned long data)
> return set_user_dscr(task, data);
>
> if (regno <= PT_MAX_PUT_REG) {
> + regno = array_index_nospec(regno, PT_MAX_PUT_REG + 1);
> ((unsigned long *)task->thread.regs)[regno] = data;
> return 0;
> }
>
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2019-01-29 17:49 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-01-24 14:01 [PATCH] powerpc/ptrace: Mitigate potential Spectre v1 Breno Leitao
2019-01-24 17:25 ` Gustavo A. R. Silva
2019-01-29 16:38 ` Breno Leitao
2019-01-29 17:00 ` Gustavo A. R. Silva
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).