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 <lei...@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; > } >