Re: [PATCH v3 4/5] powerpc/fault: Avoid heavy search_exception_tables() verification

From: Christophe Leroy
Date: Wed Dec 09 2020 - 00:35:22 EST




Le 08/12/2020 à 16:07, Christophe Leroy a écrit :


Le 08/12/2020 à 15:52, Aneesh Kumar K.V a écrit :
Christophe Leroy <christophe.leroy@xxxxxxxxxx> writes:

search_exception_tables() is an heavy operation, we have to avoid it.
When KUAP is selected, we'll know the fault has been blocked by KUAP.
Otherwise, it behaves just as if the address was already in the TLBs
and no fault was generated.

Signed-off-by: Christophe Leroy <christophe.leroy@xxxxxxxxxx>
Reviewed-by: Nicholas Piggin <npiggin@xxxxxxxxx>
---
v3: rebased
v2: Squashed with the preceeding patch which was re-ordering tests that get removed in this patch.
---
  arch/powerpc/mm/fault.c | 23 +++++++----------------
  1 file changed, 7 insertions(+), 16 deletions(-)

diff --git a/arch/powerpc/mm/fault.c b/arch/powerpc/mm/fault.c
index 3fcd34c28e10..1770b41e4730 100644
--- a/arch/powerpc/mm/fault.c
+++ b/arch/powerpc/mm/fault.c
@@ -210,28 +210,19 @@ static bool bad_kernel_fault(struct pt_regs *regs, unsigned long error_code,
          return true;
      }
-    if (!is_exec && address < TASK_SIZE && (error_code & (DSISR_PROTFAULT | DSISR_KEYFAULT)) &&
-        !search_exception_tables(regs->nip)) {
-        pr_crit_ratelimited("Kernel attempted to access user page (%lx) - exploit attempt? (uid: %d)\n",
-                    address,
-                    from_kuid(&init_user_ns, current_uid()));
-    }
-
      // Kernel fault on kernel address is bad
      if (address >= TASK_SIZE)
          return true;
-    // Fault on user outside of certain regions (eg. copy_tofrom_user()) is bad
-    if (!search_exception_tables(regs->nip))
-        return true;
-
-    // Read/write fault in a valid region (the exception table search passed
-    // above), but blocked by KUAP is bad, it can never succeed.
-    if (bad_kuap_fault(regs, address, is_write))
+    // Read/write fault blocked by KUAP is bad, it can never succeed.
+    if (bad_kuap_fault(regs, address, is_write)) {
+        pr_crit_ratelimited("Kernel attempted to %s user page (%lx) - exploit attempt? (uid: %d)\n",
+                    is_write ? "write" : "read", address,
+                    from_kuid(&init_user_ns, current_uid()));
          return true;
+    }


With this I am wondering whether the WARN() in bad_kuap_fault() is
needed. A direct access of userspace address will trigger this, whereas
previously we used bad_kuap_fault() only to identify incorrect restore
of AMR register (ie, to identify kernel bugs). Hence a WARN() there was
useful. We loose that differentiation now?

Yes, I wanted to remove the WARN(), see https://patchwork.ozlabs.org/project/linuxppc-dev/patch/cc9129bdda1dbc2f0a09cf45fece7d0b0e690784.1605541983.git.christophe.leroy@xxxxxxxxxx/
but I understood from Michael that maybe it was not a good idea, so I left it aside for now when rebasing to v3.

Yes previously we were able to differentiate between a direct access of userspace and a valid access triggering a KUAP fault, but at the cost of the heavy search_exception_tables().
The issue was reported by Nick through https://github.com/linuxppc/issues/issues/317

Should be perform the search_exception_tables() once we have hit the KUAP fault and WARN() only in that case ?

I sent out v4 which does that: only emit the warning once we know it is a KUAP fault within an uaccess routine. With that, we should be back more or less as before: warning only if we hit KUAP fault AND it is a place where a userspace access should be granted.
We are not anymore in the fast hot path, so calling search_exception_tables() there should be a performance issue.

Christophe



I was wondering also if we should keep the WARN() only when CONFIG_PPC_KUAP_DEBUG is set ?