On Thu, Apr 25, 2019 at 10:37 AM Nadav Amit [off-list ref] wrote:

quoted

On Apr 25, 2019, at 9:26 AM, Borislav Petkov [off-list ref] wrote:

On Mon, Apr 22, 2019 at 11:57:45AM -0700, Rick Edgecombe wrote:

quoted

From: Andy Lutomirski <luto@kernel.org>

Using a dedicated page-table for temporary PTEs prevents other cores
from using - even speculatively - these PTEs, thereby providing two
benefits:

(1) Security hardening: an attacker that gains kernel memory writing
abilities cannot easily overwrite sensitive data.

(2) Avoiding TLB shootdowns: the PTEs do not need to be flushed in
remote page-tables.

To do so a temporary mm_struct can be used. Mappings which are private
for this mm can be set in the userspace part of the address-space.
During the whole time in which the temporary mm is loaded, interrupts
must be disabled.

The first use-case for temporary mm struct, which will follow, is for
poking the kernel text.

[ Commit message was written by Nadav Amit ]

Cc: Kees Cook <redacted>
Cc: Dave Hansen <redacted>
Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Reviewed-by: Masami Hiramatsu <mhiramat@kernel.org>
Tested-by: Masami Hiramatsu <mhiramat@kernel.org>
Signed-off-by: Andy Lutomirski <luto@kernel.org>
Signed-off-by: Nadav Amit <redacted>
Signed-off-by: Rick Edgecombe <rick.p.edgecombe@intel.com>
---
arch/x86/include/asm/mmu_context.h | 33 ++++++++++++++++++++++++++++++
1 file changed, 33 insertions(+)

diff --git a/arch/x86/include/asm/mmu_context.h b/arch/x86/include/asm/mmu_context.h
index 19d18fae6ec6..d684b954f3c0 100644
--- a/arch/x86/include/asm/mmu_context.h
+++ b/arch/x86/include/asm/mmu_context.h

@@ -356,4 +356,37 @@ static inline unsigned long __get_current_cr3_fast(void)
     return cr3;

}

+typedef struct {
+    struct mm_struct *prev;
+} temp_mm_state_t;
+
+/*
+ * Using a temporary mm allows to set temporary mappings that are not accessible
+ * by other cores. Such mappings are needed to perform sensitive memory writes

s/cores/CPUs/g

Yeah, the concept of a thread of execution we call a CPU in the kernel,
I'd say. No matter if it is one of the hyperthreads or a single thread
in core.

quoted

+ * that override the kernel memory protections (e.g., W^X), without exposing the
+ * temporary page-table mappings that are required for these write operations to
+ * other cores.

Ditto.

quoted

Using temporary mm also allows to avoid TLB shootdowns when the

Using a ..

quoted

+ * mapping is torn down.
+ *

Nice commenting.

quoted

+ * Context: The temporary mm needs to be used exclusively by a single core. To
+ *          harden security IRQs must be disabled while the temporary mm is

                            ^
                            ,

quoted

+ *          loaded, thereby preventing interrupt handler bugs from overriding
+ *          the kernel memory protection.
+ */
+static inline temp_mm_state_t use_temporary_mm(struct mm_struct *mm)
+{
+    temp_mm_state_t state;
+
+    lockdep_assert_irqs_disabled();
+    state.prev = this_cpu_read(cpu_tlbstate.loaded_mm);
+    switch_mm_irqs_off(NULL, mm, current);
+    return state;
+}
+
+static inline void unuse_temporary_mm(temp_mm_state_t prev)
+{
+    lockdep_assert_irqs_disabled();
+    switch_mm_irqs_off(NULL, prev.prev, current);

I think this code would be more readable if you call that
temp_mm_state_t variable "temp_state" and the mm_struct pointer "mm" and
then you have:

      switch_mm_irqs_off(NULL, temp_state.mm, current);

And above you'll have:

      temp_state.mm = ...

Andy, please let me know whether you are fine with this change and I’ll
incorporate it.

I'm okay with it.