| 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
| ||
|
Message-ID: <CAPhsuW6pz6YoNRWah=nPrVRbyd=Sbh6tYDTGqP5aOxxSHw54pg@mail.gmail.com>
Date: Wed, 24 Oct 2018 14:50:52 -0700
From: Song Liu <liu.song.a23@...il.com>
To: Martin KaFai Lau <kafai@...com>
Cc: wang6495@....edu, kjlu@....edu,
Alexei Starovoitov <ast@...nel.org>,
Daniel Borkmann <daniel@...earbox.net>,
Networking <netdev@...r.kernel.org>,
open list <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2] bpf: btf: Fix a missing-check bug
On Wed, Oct 24, 2018 at 1:45 PM Martin Lau <kafai@...com> wrote:
>
> On Wed, Oct 24, 2018 at 06:22:46PM +0000, Martin Lau wrote:
> > On Wed, Oct 24, 2018 at 05:26:23PM +0000, Martin Lau wrote:
> > > On Wed, Oct 24, 2018 at 08:00:19AM -0500, Wenwen Wang wrote:
> > > > In btf_parse(), the header of the user-space btf data 'btf_data' is firstly
> > > > parsed and verified through btf_parse_hdr(). In btf_parse_hdr(), the header
> > > > is copied from user-space 'btf_data' to kernel-space 'btf->hdr' and then
> > > > verified. If no error happens during the verification process, the whole
> > > > data of 'btf_data', including the header, is then copied to 'data' in
> > > > btf_parse(). It is obvious that the header is copied twice here. More
> > > > importantly, no check is enforced after the second copy to make sure the
> > > > headers obtained in these two copies are same. Given that 'btf_data'
> > > > resides in the user space, a malicious user can race to modify the header
> > > > between these two copies. By doing so, the user can inject inconsistent
> > > > data, which can cause undefined behavior of the kernel and introduce
> > > > potential security risk.
> > btw, I am working on a patch that copies the btf_data before parsing/verifying
> > the header. That should avoid this from happening but that will
> > require a bit more code churns for the bpf branch.
> >
> It is what I have in mind:
>
>
> It is not a good idea to check the BTF header before copying the
> user btf_data. The verified header may not be the one actually
> copied to btf->data (e.g. userspace may modify the passed in
> btf_data in between). Like the one fixed in
> commit 8af03d1ae2e1 ("bpf: btf: Fix a missing check bug").
>
> This patch copies the user btf_data before parsing/verifying
> the BTF header.
>
> Fixes: 69b693f0aefa ("bpf: btf: Introduce BPF Type Format (BTF)")
> Signed-off-by: Martin KaFai Lau <kafai@...com>
Acked-by: Song Liu <songliubraving@...com>
> ---
> kernel/bpf/btf.c | 58 +++++++++++++++++++++---------------------------
> 1 file changed, 25 insertions(+), 33 deletions(-)
>
> diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
> index 378cef70341c..ee4c82667d65 100644
> --- a/kernel/bpf/btf.c
> +++ b/kernel/bpf/btf.c
> @@ -2067,56 +2067,47 @@ static int btf_check_sec_info(struct btf_verifier_env *env,
> return 0;
> }
>
> -static int btf_parse_hdr(struct btf_verifier_env *env, void __user *btf_data,
> - u32 btf_data_size)
> +static int btf_parse_hdr(struct btf_verifier_env *env)
> {
> + u32 hdr_len, hdr_copy, btf_data_size;
> const struct btf_header *hdr;
> - u32 hdr_len, hdr_copy;
> - /*
> - * Minimal part of the "struct btf_header" that
> - * contains the hdr_len.
> - */
> - struct btf_min_header {
> - u16 magic;
> - u8 version;
> - u8 flags;
> - u32 hdr_len;
> - } __user *min_hdr;
> struct btf *btf;
> int err;
>
> btf = env->btf;
> - min_hdr = btf_data;
> + btf_data_size = btf->data_size;
>
> - if (btf_data_size < sizeof(*min_hdr)) {
> + if (btf_data_size <
> + offsetof(struct btf_header, hdr_len) + sizeof(hdr->hdr_len)) {
> btf_verifier_log(env, "hdr_len not found");
> return -EINVAL;
> }
>
> - if (get_user(hdr_len, &min_hdr->hdr_len))
> - return -EFAULT;
> -
> + hdr = btf->data;
> + hdr_len = hdr->hdr_len;
> if (btf_data_size < hdr_len) {
> btf_verifier_log(env, "btf_header not found");
> return -EINVAL;
> }
>
> - err = bpf_check_uarg_tail_zero(btf_data, sizeof(btf->hdr), hdr_len);
> - if (err) {
> - if (err == -E2BIG)
> - btf_verifier_log(env, "Unsupported btf_header");
> - return err;
> + /* Ensure the unsupported header fields are zero */
> + if (hdr_len > sizeof(btf->hdr)) {
> + u8 *expected_zero = btf->data + sizeof(btf->hdr);
> + u8 *end = btf->data + hdr_len;
> +
> + for (; expected_zero < end; expected_zero++) {
> + if (*expected_zero) {
> + btf_verifier_log(env, "Unsupported btf_header");
> + return -E2BIG;
> + }
> + }
> }
>
> hdr_copy = min_t(u32, hdr_len, sizeof(btf->hdr));
> - if (copy_from_user(&btf->hdr, btf_data, hdr_copy))
> - return -EFAULT;
> + memcpy(&btf->hdr, btf->data, hdr_copy);
>
> hdr = &btf->hdr;
>
> - if (hdr->hdr_len != hdr_len)
> - return -EINVAL;
> -
> btf_verifier_log_hdr(env, btf_data_size);
>
> if (hdr->magic != BTF_MAGIC) {
> @@ -2186,10 +2177,6 @@ static struct btf *btf_parse(void __user *btf_data, u32 btf_data_size,
> }
> env->btf = btf;
>
> - err = btf_parse_hdr(env, btf_data, btf_data_size);
> - if (err)
> - goto errout;
> -
> data = kvmalloc(btf_data_size, GFP_KERNEL | __GFP_NOWARN);
> if (!data) {
> err = -ENOMEM;
> @@ -2198,13 +2185,18 @@ static struct btf *btf_parse(void __user *btf_data, u32 btf_data_size,
>
> btf->data = data;
> btf->data_size = btf_data_size;
> - btf->nohdr_data = btf->data + btf->hdr.hdr_len;
>
> if (copy_from_user(data, btf_data, btf_data_size)) {
> err = -EFAULT;
> goto errout;
> }
>
> + err = btf_parse_hdr(env);
> + if (err)
> + goto errout;
> +
> + btf->nohdr_data = btf->data + btf->hdr.hdr_len;
> +
> err = btf_parse_str_sec(env);
> if (err)
> goto errout;
> --
> 2.17.1
>
>
Powered by blists - more mailing lists