Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: quoted-printable
Mime-Version: 1.0 (1.0)
Subject: Re: [PATCH bpf-next] bpf: Make trampolines W^X
From:   Andy Lutomirski <luto@amacapital.net>
In-Reply-To: <20200103234725.22846-1-kpsingh@chromium.org>
Cc:     linux-kernel@vger.kernel.org, bpf@vger.kernel.org, x86@kernel.org,
        linux-security-module@vger.kernel.org,
        Kees Cook <keescook@chromium.org>,
        "David S. Miller" <davem@davemloft.net>,
        Alexey Kuznetsov <kuznet@ms2.inr.ac.ru>,
        Hideaki YOSHIFUJI <yoshfuji@linux-ipv6.org>,
        Alexei Starovoitov <ast@kernel.org>,
        Daniel Borkmann <daniel@iogearbox.net>,
        Martin KaFai Lau <kafai@fb.com>,
        Song Liu <songliubraving@fb.com>, Yonghong Song <yhs@fb.com>,
        Andrii Nakryiko <andriin@fb.com>,
        Thomas Garnier <thgarnie@chromium.org>,
        Florent Revest <revest@chromium.org>,
        Brendan Jackman <jackmanb@chromium.org>,
        Jann Horn <jannh@google.com>,
        Matthew Garrett <mjg59@google.com>,
        Michael Halcrow <mhalcrow@google.com>
Date:   Sat, 4 Jan 2020 09:49:10 +0900
Message-Id: <F25C9071-A7A7-4221-BC49-A769E1677EE1@amacapital.net>
References: <20200103234725.22846-1-kpsingh@chromium.org>
To:     KP Singh <kpsingh@chromium.org>,
        Rick Edgecombe <rick.p.edgecombe@intel.com>
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk


> On Jan 4, 2020, at 8:47 AM, KP Singh <kpsingh@chromium.org> wrote:
>=20
> =EF=BB=BFFrom: KP Singh <kpsingh@google.com>
>=20
> 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.
>=20
> 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?

>=20
> - 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=E2=80=99s worth noting that this whole approach is subo=
ptimal: the =E2=80=9Cmodule=E2=80=9D allocator should really be returning a l=
ist of pages to be written (not at the final address!) with the actual execu=
table mapping to be materialized later, but that=E2=80=99s a bigger project t=
hat you=E2=80=99re welcome to ignore for now.  (Concretely, it should produc=
e a vmap address with backing pages but with the vmap alias either entirely u=
nmapped or read-only. A subsequent healer would, all at once, make the direc=
t 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=E2=80=99s very little excuse for doing two IPI flushes whe=
n one would suffice.

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