Re: [PATCH] powerpc/ptrace: Mitigate potential Spectre v1
From: Gustavo A. R. Silva <hidden>
Date: 2019-01-24 17:50:51
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.
quoted hunk ↗ jump to hunk
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.
quoted hunk ↗ jump to hunk
*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