Re: [PATCH] powerpc/ptrace: Mitigate potential Spectre v1

["Gustavo A. R. Silva" <gustavo@embeddedor.com>]

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