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

From: "Edgecombe, Rick P" <rick.p.edgecombe@intel.com>
To: "kpsingh@chromium.org" <kpsingh@chromium.org>,
	"luto@amacapital.net" <luto@amacapital.net>
Cc: "songliubraving@fb.com" <songliubraving@fb.com>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	"bpf@vger.kernel.org" <bpf@vger.kernel.org>,
	"keescook@chromium.org" <keescook@chromium.org>,
	"ast@kernel.org" <ast@kernel.org>,
	"daniel@iogearbox.net" <daniel@iogearbox.net>,
	"kuznet@ms2.inr.ac.ru" <kuznet@ms2.inr.ac.ru>,
	"jannh@google.com" <jannh@google.com>,
	"mjg59@google.com" <mjg59@google.com>,
	"thgarnie@chromium.org" <thgarnie@chromium.org>,
	"linux-security-module@vger.kernel.org" 
	<linux-security-module@vger.kernel.org>,
	"x86@kernel.org" <x86@kernel.org>,
	"revest@chromium.org" <revest@chromium.org>,
	"jackmanb@chromium.org" <jackmanb@chromium.org>,
	"kafai@fb.com" <kafai@fb.com>, "yhs@fb.com" <yhs@fb.com>,
	"davem@davemloft.net" <davem@davemloft.net>,
	"yoshfuji@linux-ipv6.org" <yoshfuji@linux-ipv6.org>,
	"mhalcrow@google.com" <mhalcrow@google.com>,
	"andriin@fb.com" <andriin@fb.com>
Subject: Re: [PATCH bpf-next] bpf: Make trampolines W^X
Date: Mon, 6 Jan 2020 22:25:27 +0000	[thread overview]
Message-ID: <21bf6bb46544eab79e792980f82520f8fbdae9b5.camel@intel.com> (raw)
In-Reply-To: <F25C9071-A7A7-4221-BC49-A769E1677EE1@amacapital.net>

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.

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