[PATCH] powerpc/ptrace: Mitigate potential Spectre v1

[Gustavo A. R. Silva]

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 at 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;
>  	}
>