linux-kernel - Re: [PATCH bpf-next] bpf: Make trampolines W^X

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <F25C9071-A7A7-4221-BC49-A769E1677EE1@amacapital.net>
Date:   Sat, 4 Jan 2020 09:49:10 +0900
From:   Andy Lutomirski <luto@...capital.net>
To:     KP Singh <kpsingh@...omium.org>,
        Rick Edgecombe <rick.p.edgecombe@...el.com>
Cc:     linux-kernel@...r.kernel.org, bpf@...r.kernel.org, x86@...nel.org,
        linux-security-module@...r.kernel.org,
        Kees Cook <keescook@...omium.org>,
        "David S. Miller" <davem@...emloft.net>,
        Alexey Kuznetsov <kuznet@....inr.ac.ru>,
        Hideaki YOSHIFUJI <yoshfuji@...ux-ipv6.org>,
        Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        Martin KaFai Lau <kafai@...com>,
        Song Liu <songliubraving@...com>, Yonghong Song <yhs@...com>,
        Andrii Nakryiko <andriin@...com>,
        Thomas Garnier <thgarnie@...omium.org>,
        Florent Revest <revest@...omium.org>,
        Brendan Jackman <jackmanb@...omium.org>,
        Jann Horn <jannh@...gle.com>,
        Matthew Garrett <mjg59@...gle.com>,
        Michael Halcrow <mhalcrow@...gle.com>
Subject: Re: [PATCH bpf-next] bpf: Make trampolines W^X

> On Jan 4, 2020, at 8:47 AM, KP Singh <kpsingh@...omium.org> wrote:
> 
> From: KP Singh <kpsingh@...gle.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.