Re: [PATCH 1/2] powerpc/64s: Fix entry flush patching w/strict RWX & hash

2021-05-15 Thread Michael Ellerman
On Fri, 14 May 2021 00:07:59 +1000, Michael Ellerman wrote:
> The entry flush mitigation can be enabled/disabled at runtime. When this
> happens it results in the kernel patching its own instructions to
> enable/disable the mitigation sequence.
> 
> With strict kernel RWX enabled instruction patching happens via a
> secondary mapping of the kernel text, so that we don't have to make the
> primary mapping writable. With the hash MMU this leads to a hash fault,
> which causes us to execute the exception entry which contains the entry
> flush mitigation.
> 
> [...]

Applied to powerpc/fixes.

[1/2] powerpc/64s: Fix entry flush patching w/strict RWX & hash
  https://git.kernel.org/powerpc/c/49b39ec248af863781a13aa6d81c5f69a2928094
[2/2] powerpc/64s: Fix stf mitigation patching w/strict RWX & hash
  https://git.kernel.org/powerpc/c/5b48ba2fbd77bc68feebd336ffad5ff166782bde

cheers


[PATCH 1/2] powerpc/64s: Fix entry flush patching w/strict RWX & hash

2021-05-13 Thread Michael Ellerman
The entry flush mitigation can be enabled/disabled at runtime. When this
happens it results in the kernel patching its own instructions to
enable/disable the mitigation sequence.

With strict kernel RWX enabled instruction patching happens via a
secondary mapping of the kernel text, so that we don't have to make the
primary mapping writable. With the hash MMU this leads to a hash fault,
which causes us to execute the exception entry which contains the entry
flush mitigation.

This means we end up executing the entry flush in a semi-patched state,
ie. after we have patched the first instruction but before we patch the
second or third instruction of the sequence.

On machines with updated firmware the entry flush is a series of special
nops, and it's safe to to execute in a semi-patched state.

However when using the fallback flush the sequence is mflr/branch/mtlr,
and so it's not safe to execute if we have patched out the mflr but not
the other two instructions. Doing so leads to us corrputing LR, leading
to an oops, for example:

  # echo 0 > /sys/kernel/debug/powerpc/entry_flush
  kernel tried to execute exec-protected page (c2971000) - exploit 
attempt? (uid: 0)
  BUG: Unable to handle kernel instruction fetch
  Faulting instruction address: 0xc2971000
  Oops: Kernel access of bad area, sig: 11 [#1]
  LE PAGE_SIZE=64K MMU=Hash SMP NR_CPUS=2048 NUMA pSeries
  CPU: 0 PID: 2215 Comm: bash Not tainted 5.13.0-rc1-00010-gda3bb206c9ce #1
  NIP:  c2971000 LR: c2971000 CTR: c0120c40
  REGS: c00013243840 TRAP: 0400   Not tainted  
(5.13.0-rc1-00010-gda3bb206c9ce)
  MSR:  800010009033   CR: 48428482  XER: 
  ...
  NIP  0xc2971000
  LR   0xc2971000
  Call Trace:
do_patch_instruction+0xc4/0x340 (unreliable)
do_entry_flush_fixups+0x100/0x3b0
entry_flush_set+0x50/0xe0
simple_attr_write+0x160/0x1a0
full_proxy_write+0x8c/0x110
vfs_write+0xf0/0x340
ksys_write+0x84/0x140
system_call_exception+0x164/0x2d0
system_call_common+0xec/0x278

The simplest fix is to change the order in which we patch the
instructions, so that the sequence is always safe to execute. For the
non-fallback flushes it doesn't matter what order we patch in.

Fixes: bd573a81312f ("powerpc/mm/64s: Allow STRICT_KERNEL_RWX again")
Signed-off-by: Michael Ellerman 
---
 arch/powerpc/lib/feature-fixups.c | 56 ++-
 1 file changed, 40 insertions(+), 16 deletions(-)

diff --git a/arch/powerpc/lib/feature-fixups.c 
b/arch/powerpc/lib/feature-fixups.c
index 0aefa6a4a259..b49bb41e3ec5 100644
--- a/arch/powerpc/lib/feature-fixups.c
+++ b/arch/powerpc/lib/feature-fixups.c
@@ -325,6 +325,28 @@ static int __do_entry_flush_fixups(void *data)
if (types & L1D_FLUSH_MTTRIG)
instrs[i++] = 0x7c12dba6; /* mtspr TRIG2,r0 (SPR #882) */
 
+   /*
+* If we're patching in or out the fallback flush we need to be careful 
about the
+* order in which we patch instructions. That's because it's possible 
we could
+* take a page fault after patching one instruction, so the sequence of
+* instructions must be safe even in a half patched state.
+*
+* To make that work, when patching in the fallback flush we patch in 
this order:
+*  - the mflr  (dest)
+*  - the mtlr  (dest + 2)
+*  - the branch(dest + 1)
+*
+* That ensures the sequence is safe to execute at any point. In 
contrast if we
+* patch the mtlr last, it's possible we could return from the branch 
and not
+* restore LR, leading to a crash later.
+*
+* When patching out the fallback flush (either with nops or another 
flush type),
+* we patch in this order:
+*  - the branch(dest + 1)
+*  - the mtlr  (dest + 2)
+*  - the mflr  (dest)
+*/
+
start = PTRRELOC(&__start___entry_flush_fixup);
end = PTRRELOC(&__stop___entry_flush_fixup);
for (i = 0; start < end; start++, i++) {
@@ -332,15 +354,16 @@ static int __do_entry_flush_fixups(void *data)
 
pr_devel("patching dest %lx\n", (unsigned long)dest);
 
-   patch_instruction((struct ppc_inst *)dest, ppc_inst(instrs[0]));
-
-   if (types == L1D_FLUSH_FALLBACK)
-   patch_branch((struct ppc_inst *)(dest + 1), (unsigned 
long)_flush_fallback,
-BRANCH_SET_LINK);
-   else
+   if (types == L1D_FLUSH_FALLBACK) {
+   patch_instruction((struct ppc_inst *)dest, 
ppc_inst(instrs[0]));
+   patch_instruction((struct ppc_inst *)(dest + 2), 
ppc_inst(instrs[2]));
+   patch_branch((struct ppc_inst *)(dest + 1),
+(unsigned long)_flush_fallback, 
BRANCH_SET_LINK);
+   } else {