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: <cdd157ef011efda92c9434f76141fc3aef174d85.camel@intel.com>
Date:   Tue, 7 Jan 2020 19:01:44 +0000
From:   "Edgecombe, Rick P" <rick.p.edgecombe@...el.com>
To:     "luto@...capital.net" <luto@...capital.net>
CC:     "songliubraving@...com" <songliubraving@...com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "bpf@...r.kernel.org" <bpf@...r.kernel.org>,
        "keescook@...omium.org" <keescook@...omium.org>,
        "jeyu@...nel.org" <jeyu@...nel.org>,
        "ast@...nel.org" <ast@...nel.org>,
        "kuznet@....inr.ac.ru" <kuznet@....inr.ac.ru>,
        "daniel@...earbox.net" <daniel@...earbox.net>,
        "mjg59@...gle.com" <mjg59@...gle.com>,
        "thgarnie@...omium.org" <thgarnie@...omium.org>,
        "kpsingh@...omium.org" <kpsingh@...omium.org>,
        "linux-security-module@...r.kernel.org" 
        <linux-security-module@...r.kernel.org>,
        "x86@...nel.org" <x86@...nel.org>,
        "revest@...omium.org" <revest@...omium.org>,
        "jannh@...gle.com" <jannh@...gle.com>,
        "namit@...are.com" <namit@...are.com>,
        "jackmanb@...omium.org" <jackmanb@...omium.org>,
        "kafai@...com" <kafai@...com>, "yhs@...com" <yhs@...com>,
        "davem@...emloft.net" <davem@...emloft.net>,
        "yoshfuji@...ux-ipv6.org" <yoshfuji@...ux-ipv6.org>,
        "mhalcrow@...gle.com" <mhalcrow@...gle.com>,
        "andriin@...com" <andriin@...com>
Subject: Re: [PATCH bpf-next] bpf: Make trampolines W^X

CC Nadav and Jessica.

On Mon, 2020-01-06 at 15:36 -1000, Andy Lutomirski wrote:
> > On Jan 6, 2020, at 12:25 PM, Edgecombe, Rick P <rick.p.edgecombe@...el.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@...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.
> > 
> > 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?

> > 
> > 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.