lkml.org 
[lkml]   [2020]   [Jan]   [6]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
SubjectRe: [PATCH bpf-next] bpf: Make trampolines W^X
Date

> On Jan 6, 2020, at 12:25 PM, Edgecombe, Rick P <rick.p.edgecombe@intel.com> wrote:
>
> On Sat, 2020-01-04 at 09:49 +0900, Andy Lutomirski wrote:
>>>> On Jan 4, 2020, at 8:47 AM, KP Singh <kpsingh@chromium.org> wrote:
>>>
>>> From: KP Singh <kpsingh@google.com>
>>>
>>> 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?
>>
>>>
>>> - 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?
>>
>>> - Mark the memory as read/write (set_memory_rw)
>>
>> Probably harmless, but see above about fixing it.
>>
>>> - 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.)
>>> - 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.

>
> 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?
\
 
 \ /
  Last update: 2020-01-07 02:37    [W:0.112 / U:0.080 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site