netdev - Re: [PATCH bpf-next v3] bpf: Lift hashtab key

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] [day] [month] [year] [list]

Message-ID: <20201027132542.GA2902@der-flo.net>
Date:   Tue, 27 Oct 2020 14:25:42 +0100
From:   Florian Lehner <dev@...-flo.net>
To:     Andrii Nakryiko <andrii.nakryiko@...il.com>
Cc:     ast@...nel.org, bpf@...r.kernel.org, daniel@...earbox.net,
        dev@...-flo.net, john.fastabend@...il.com, netdev@...r.kernel.org
Subject: Re: [PATCH bpf-next v3] bpf: Lift hashtab key_size limit

On Mon, Oct 26, 2020 at 04:07:58PM -0700, Andrii Nakryiko wrote:
> >
> > Signed-off-by: Florian Lehner <dev@...-flo.net>
> > ---
> 
> You dropped the ack from John, btw.

I was not sure if it is ok to keep the ACK for an updated patch. So I
did not include it.

> >  kernel/bpf/hashtab.c                          | 16 +++----
> >  .../selftests/bpf/prog_tests/hash_large_key.c | 28 ++++++++++++
> >  .../selftests/bpf/progs/test_hash_large_key.c | 45 +++++++++++++++++++
> >  tools/testing/selftests/bpf/test_maps.c       |  2 +-
> >  4 files changed, 79 insertions(+), 12 deletions(-)
> >  create mode 100644 tools/testing/selftests/bpf/prog_tests/hash_large_key.c
> >  create mode 100644 tools/testing/selftests/bpf/progs/test_hash_large_key.c
> >
> > diff --git a/kernel/bpf/hashtab.c b/kernel/bpf/hashtab.c
> > index 1815e97d4c9c..10097d6bcc35 100644
> > --- a/kernel/bpf/hashtab.c
> > +++ b/kernel/bpf/hashtab.c
> > @@ -390,17 +390,11 @@ static int htab_map_alloc_check(union bpf_attr *attr)
> >             attr->value_size == 0)
> >                 return -EINVAL;
> >
> > -       if (attr->key_size > MAX_BPF_STACK)
> > -               /* eBPF programs initialize keys on stack, so they cannot be
> > -                * larger than max stack size
> > -                */
> > -               return -E2BIG;
> > -
> > -       if (attr->value_size >= KMALLOC_MAX_SIZE -
> > -           MAX_BPF_STACK - sizeof(struct htab_elem))
> > -               /* if value_size is bigger, the user space won't be able to
> > -                * access the elements via bpf syscall. This check also makes
> > -                * sure that the elem_size doesn't overflow and it's
> > +       if ((attr->key_size + attr->value_size) >= KMALLOC_MAX_SIZE -
> 
> key_size+value_size can overflow, can't it? So probably want to cast
> to (size_t) or __u64?
> 

I will add this cast to u64.

> > +           sizeof(struct htab_elem))
> > +               /* if key_size + value_size is bigger, the user space won't be
> > +                * able to access the elements via bpf syscall. This check
> > +                * also makes sure that the elem_size doesn't overflow and it's
> >                  * kmalloc-able later in htab_map_update_elem()
> >                  */
> >                 return -E2BIG;
> > diff --git a/tools/testing/selftests/bpf/prog_tests/hash_large_key.c b/tools/testing/selftests/bpf/prog_tests/hash_large_key.c
> > new file mode 100644
> > index 000000000000..962f56060b76
> > --- /dev/null
> > +++ b/tools/testing/selftests/bpf/prog_tests/hash_large_key.c
> > @@ -0,0 +1,28 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +// Copyright (c) 2020 Florian Lehner
> > +
> > +#include <test_progs.h>
> > +
> > +void test_hash_large_key(void)
> > +{
> > +       const char *file = "./test_hash_large_key.o";
> > +       int prog_fd, map_fd[2];
> > +       struct bpf_object *obj = NULL;
> > +       int err = 0;
> > +
> > +       err = bpf_prog_load(file, BPF_PROG_TYPE_CGROUP_SKB, &obj, &prog_fd);
> 
> Please utilize the BPF skeleton in the new tests.
> 

I will update the test and utilize the BPF skeleton.

> > +       if (CHECK_FAIL(err)) {
> > +               printf("test_hash_large_key: bpf_prog_load errno %d", errno);
> > +               goto close_prog;
> > +       }
> > +
> > +       map_fd[0] = bpf_find_map(__func__, obj, "hash_map");
> > +       if (CHECK_FAIL(map_fd[0] < 0))
> > +               goto close_prog;
> > +       map_fd[1] = bpf_find_map(__func__, obj, "key_map");
> > +       if (CHECK_FAIL(map_fd[1] < 0))
> > +               goto close_prog;
> 
> You are not really checking much here.
> 
> Why don't you check that the value was set to 42 here? Consider also
> using big global variables as your key and value. You can specify them
> from user-space with BPF skeleton easily to any custom value (not just
> zeroes).
> 
> 
> > +
> > +close_prog:
> > +       bpf_object__close(obj);
> > +}
> 
> [...]