Re: [PATCH 06/17] x86/alternative: use temporary mm for text poking

On January 17, 2019 1:43:54 PM PST, Nadav Amit [off-list ref] wrote:

quoted

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

wrote:

quoted

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

wrote:

quoted

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

quoted

quoted

quoted

PTEs in the fixmap. These PTEs might be used to rewrite the kernel

code

quoted

quoted

quoted

from other cores accidentally or maliciously, if an attacker gains

the

quoted

quoted

quoted

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,

quoted

quoted

quoted

+        * as it also flushes the corresponding "user" address

spaces, which

quoted

quoted

quoted

+        * does not exist.
+        *
+        * Poking, however, is already very inefficient since it

does not try to

quoted

quoted

quoted

+        * batch updates, so we ignore this problem for the time

being.

quoted

quoted

quoted

+        *
+        * Since the PTEs do not exist in other kernel

address-spaces, we do

quoted

quoted

quoted

+        * not use __flush_tlb_one_kernel(), which when PTI is on

would cause

quoted

quoted

quoted

+        * more unwarranted TLB flushes.
+        *
+        * There is a slight anomaly here: the PTE is a

supervisor-only and

quoted

quoted

quoted

+        * (potentially) global and we use __flush_tlb_one_user()

but this

quoted

quoted

quoted

+        * should be fine.
+        */
+       __flush_tlb_one_user(poking_addr);
+       if (cross_page_boundary) {
+               pte_clear(poking_mm, poking_addr + PAGE_SIZE, ptep

+ 1);

quoted

quoted

quoted

+               __flush_tlb_one_user(poking_addr + PAGE_SIZE);
+       }

In principle, another CPU could still have the old translation. 

Your

quoted

quoted

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

quoted

quoted

with IRQs off.  Hmm.  I think you should add an inc_mm_tlb_gen()

here.

quoted

quoted

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

quoted

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

You can just disable the global bit at the top level, obviously.

This approach also should make it far easier to do batching if desired.
-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.