On Jan 17, 2019, at 12:47 PM, Andy Lutomirski [off-list ref] wrote:

On Thu, Jan 17, 2019 at 12:27 PM Andy Lutomirski [off-list ref] wrote:

quoted

On Wed, Jan 16, 2019 at 4:33 PM Rick Edgecombe
[off-list ref] wrote:

quoted

From: Nadav Amit <redacted>

text_poke() can potentially compromise the security as it sets temporary
PTEs in the fixmap. These PTEs might be used to rewrite the kernel code
from other cores accidentally or maliciously, if an attacker gains the
ability to write onto kernel memory.

i think this may be sufficient, but barely.

quoted

+       pte_clear(poking_mm, poking_addr, ptep);
+
+       /*
+        * __flush_tlb_one_user() performs a redundant TLB flush when PTI is on,
+        * as it also flushes the corresponding "user" address spaces, which
+        * does not exist.
+        *
+        * Poking, however, is already very inefficient since it does not try to
+        * batch updates, so we ignore this problem for the time being.
+        *
+        * Since the PTEs do not exist in other kernel address-spaces, we do
+        * not use __flush_tlb_one_kernel(), which when PTI is on would cause
+        * more unwarranted TLB flushes.
+        *
+        * There is a slight anomaly here: the PTE is a supervisor-only and
+        * (potentially) global and we use __flush_tlb_one_user() but this
+        * should be fine.
+        */
+       __flush_tlb_one_user(poking_addr);
+       if (cross_page_boundary) {
+               pte_clear(poking_mm, poking_addr + PAGE_SIZE, ptep + 1);
+               __flush_tlb_one_user(poking_addr + PAGE_SIZE);
+       }

In principle, another CPU could still have the old translation.  Your
mutex probably makes this impossible, but it makes me nervous.
Ideally you'd use flush_tlb_mm_range(), but I guess you can't do that
with IRQs off.  Hmm.  I think you should add an inc_mm_tlb_gen() here.
Arguably, if you did that, you could omit the flushes, but maybe
that's silly.

If we start getting new users of use_temporary_mm(), we should give
some serious thought to the SMP semantics.

Also, you're using PAGE_KERNEL.  Please tell me that the global bit
isn't set in there.

Much better solution: do unuse_temporary_mm() and *then*
flush_tlb_mm_range().  This is entirely non-sketchy and should be just
about optimal, too.

This solution sounds nice and clean. The fact the global-bit was set didn’t
matter before (since __flush_tlb_one_user would get rid of it no matter
what), but would matter now, so I’ll change it too.

Thanks!

Nadav