CC Nadav and Jessica.

On Mon, 2020-01-06 at 15:36 -1000, Andy Lutomirski wrote:

quoted

On Jan 6, 2020, at 12:25 PM, Edgecombe, Rick P [off-list ref]
wrote:

On Sat, 2020-01-04 at 09:49 +0900, Andy Lutomirski wrote:

quoted

quoted

quoted

On Jan 4, 2020, at 8:47 AM, KP Singh [off-list ref] wrote:

From: KP Singh [off-list ref]

The image for the BPF trampolines is allocated with
bpf_jit_alloc_exe_page which marks this allocated page executable. This
means that the allocated memory is W and X at the same time making it
susceptible to WX based attacks.

Since the allocated memory is shared between two trampolines (the
current and the next), 2 pages must be allocated to adhere to W^X and
the following sequence is obeyed where trampolines are modified:

Can we please do better rather than piling garbage on top of garbage?

quoted

- Mark memory as non executable (set_memory_nx). While module_alloc for
x86 allocates the memory as PAGE_KERNEL and not PAGE_KERNEL_EXEC, not
all implementations of module_alloc do so

How about fixing this instead?

quoted

- Mark the memory as read/write (set_memory_rw)

Probably harmless, but see above about fixing it.

quoted

- Modify the trampoline

Seems reasonable. It’s worth noting that this whole approach is
suboptimal:
the “module” allocator should really be returning a list of pages to be
written (not at the final address!) with the actual executable mapping to
be
materialized later, but that’s a bigger project that you’re welcome to
ignore
for now.  (Concretely, it should produce a vmap address with backing pages
but
with the vmap alias either entirely unmapped or read-only. A subsequent
healer
would, all at once, make the direct map pages RO or not-present and make
the
vmap alias RX.)

quoted

- Mark the memory as read-only (set_memory_ro)
- Mark the memory as executable (set_memory_x)

No, thanks. There’s very little excuse for doing two IPI flushes when one
would suffice.

As far as I know, all architectures can do this with a single flush
without
races  x86 certainly can. The module freeing code gets this sequence
right.
Please reuse its mechanism or, if needed, export the relevant interfaces.

So if I understand this right, some trampolines have been added that are
currently set as RWX at modification time AND left that way during runtime?
The
discussion on the order of set_memory_() calls in the commit message made me
think that this was just a modification time thing at first.

I’m not sure what the status quo is.

We really ought to have a genuinely good API for allocation and initialization
of text.  We can do so much better than set_memory_blahblah.

FWIW, I have some ideas about making kernel flushes cheaper. It’s currently
blocked on finding some time and on tglx’s irqtrace work.

Makes sense to me. I guess there are 6 types of text allocations now:
 - These two BPF trampolines
 - BPF JITs
 - Modules
 - Kprobes
 - Ftrace

All doing (or should be doing) pretty much the same thing. I believe Jessica had
said at one point that she didn't like all the other features using
module_alloc() as it was supposed to be just for real modules. Where would the
API live?

quoted

Also, is there a reason you couldn't use text_poke() to modify the
trampoline
with a single flush?

Does text_poke to an IPI these days?

I don't think so since the RW mapping is just on a single CPU. That was one of
the benefits of the temporary mm struct based thing Nadav did. I haven't looked
into PeterZ's changes though.