| Message ID | 20190319203239.gl46fxnfz6gzeeic@linutronix.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ARM: mm: harden branch predictor before opening interrupts during fault | expand |
On 2019-03-19 21:32:39 [+0100], To linux-arm-kernel@lists.infradead.org wrote: > On non-LPAE systems a write to 0xbffffff0 (modules area) from userland > results in: > | BUG: using smp_processor_id() in preemptible [00000000] code: mem-tc/521 > | caller is __do_user_fault.constprop.2+0x4c/0x74 > | CPU: 1 PID: 521 Comm: mem-tc Not tainted 5.1.0-rc1 #4 > | [<c04614e4>] (debug_smp_processor_id) from [<c0116378>] (__do_user_fault.constprop.2+0x4c/0x74) > | [<c0116378>] (__do_user_fault.constprop.2) from [<c011668c>] (do_page_fault+0x278/0x37c) > | [<c011668c>] (do_page_fault) from [<c0116904>] (do_DataAbort+0x3c/0xa8) > | [<c0116904>] (do_DataAbort) from [<c0101e1c>] (__dabt_usr+0x3c/0x40) > > Move harden_branch_predictor() from __do_user_fault() to its both > callers (do_bad_area() and do_page_fault()). The invocation in > do_page_fault() is added before interrupst are enabled. The invocation > in do_bad_area() is added just before __do_user_fault() is invoked. In 20190216113338.irr5j4ukhpwngval@shell.armlinux.org.uk Russel complained that I am opening a window for branch predictor attacks that he tried to close. This is no longer the case because harden_branch_predictor() is now in do_page_fault() and do_bad_area(). So is this still obviously wrong and I don't see it? > Fixes: f5fe12b1eaee2 ("ARM: spectre-v2: harden user aborts in kernel space") > Reported-by: Bernd Edlinger <bernd.edlinger@hotmail.de> > Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de> > --- > arch/arm/mm/fault.c | 12 +++++++----- > 1 file changed, 7 insertions(+), 5 deletions(-) > > diff --git a/arch/arm/mm/fault.c b/arch/arm/mm/fault.c > index 58f69fa07df95..7adff8eb8f3d2 100644 > --- a/arch/arm/mm/fault.c > +++ b/arch/arm/mm/fault.c > @@ -161,9 +161,6 @@ __do_user_fault(struct task_struct *tsk, unsigned long addr, > unsigned int fsr, unsigned int sig, int code, > struct pt_regs *regs) > { > - if (addr > TASK_SIZE) > - harden_branch_predictor(); > - > #ifdef CONFIG_DEBUG_USER > if (((user_debug & UDBG_SEGV) && (sig == SIGSEGV)) || > ((user_debug & UDBG_BUS) && (sig == SIGBUS))) { > @@ -195,10 +192,13 @@ void do_bad_area(unsigned long addr, unsigned int fsr, struct pt_regs *regs) > * If we are in kernel mode at this point, we > * have no context to handle this fault with. > */ > - if (user_mode(regs)) > + if (user_mode(regs)) { > + if (addr > TASK_SIZE) > + harden_branch_predictor(); > __do_user_fault(tsk, addr, fsr, SIGSEGV, SEGV_MAPERR, regs); > - else > + } else { > __do_kernel_fault(mm, addr, fsr, regs); > + } > } > > #ifdef CONFIG_MMU > @@ -272,6 +272,8 @@ do_page_fault(unsigned long addr, unsigned int fsr, struct pt_regs *regs) > tsk = current; > mm = tsk->mm; > > + if (addr > TASK_SIZE && user_mode(regs)) > + harden_branch_predictor(); > /* Enable interrupts if they were enabled in the parent context. */ > if (interrupts_enabled(regs)) > local_irq_enable(); Sebastian
diff --git a/arch/arm/mm/fault.c b/arch/arm/mm/fault.c index 58f69fa07df95..7adff8eb8f3d2 100644 --- a/arch/arm/mm/fault.c +++ b/arch/arm/mm/fault.c @@ -161,9 +161,6 @@ __do_user_fault(struct task_struct *tsk, unsigned long addr, unsigned int fsr, unsigned int sig, int code, struct pt_regs *regs) { - if (addr > TASK_SIZE) - harden_branch_predictor(); - #ifdef CONFIG_DEBUG_USER if (((user_debug & UDBG_SEGV) && (sig == SIGSEGV)) || ((user_debug & UDBG_BUS) && (sig == SIGBUS))) { @@ -195,10 +192,13 @@ void do_bad_area(unsigned long addr, unsigned int fsr, struct pt_regs *regs) * If we are in kernel mode at this point, we * have no context to handle this fault with. */ - if (user_mode(regs)) + if (user_mode(regs)) { + if (addr > TASK_SIZE) + harden_branch_predictor(); __do_user_fault(tsk, addr, fsr, SIGSEGV, SEGV_MAPERR, regs); - else + } else { __do_kernel_fault(mm, addr, fsr, regs); + } } #ifdef CONFIG_MMU @@ -272,6 +272,8 @@ do_page_fault(unsigned long addr, unsigned int fsr, struct pt_regs *regs) tsk = current; mm = tsk->mm; + if (addr > TASK_SIZE && user_mode(regs)) + harden_branch_predictor(); /* Enable interrupts if they were enabled in the parent context. */ if (interrupts_enabled(regs)) local_irq_enable();
On non-LPAE systems a write to 0xbffffff0 (modules area) from userland results in: | BUG: using smp_processor_id() in preemptible [00000000] code: mem-tc/521 | caller is __do_user_fault.constprop.2+0x4c/0x74 | CPU: 1 PID: 521 Comm: mem-tc Not tainted 5.1.0-rc1 #4 | [<c04614e4>] (debug_smp_processor_id) from [<c0116378>] (__do_user_fault.constprop.2+0x4c/0x74) | [<c0116378>] (__do_user_fault.constprop.2) from [<c011668c>] (do_page_fault+0x278/0x37c) | [<c011668c>] (do_page_fault) from [<c0116904>] (do_DataAbort+0x3c/0xa8) | [<c0116904>] (do_DataAbort) from [<c0101e1c>] (__dabt_usr+0x3c/0x40) Move harden_branch_predictor() from __do_user_fault() to its both callers (do_bad_area() and do_page_fault()). The invocation in do_page_fault() is added before interrupst are enabled. The invocation in do_bad_area() is added just before __do_user_fault() is invoked. Fixes: f5fe12b1eaee2 ("ARM: spectre-v2: harden user aborts in kernel space") Reported-by: Bernd Edlinger <bernd.edlinger@hotmail.de> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de> --- arch/arm/mm/fault.c | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-)