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

Message-ID: <CAEf4BzY-bNN7fx2eAvRBq89pDHptEqoftgSSF=0dv_GgeNACvw@mail.gmail.com>
Date:   Mon, 26 Oct 2020 16:07:58 -0700
From:   Andrii Nakryiko <andrii.nakryiko@...il.com>
To:     Florian Lehner <dev@...-flo.net>
Cc:     bpf <bpf@...r.kernel.org>, Networking <netdev@...r.kernel.org>,
        Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        john fastabend <john.fastabend@...il.com>
Subject: Re: [PATCH bpf-next v3] bpf: Lift hashtab key_size limit

On Sat, Oct 24, 2020 at 7:10 AM Florian Lehner <dev@...-flo.net> wrote:
>
> Currently key_size of hashtab is limited to MAX_BPF_STACK.
> As the key of hashtab can also be a value from a per cpu map it can be
> larger than MAX_BPF_STACK.
>
> The use-case for this patch originates to implement allow/disallow
> lists for files and file paths. The maximum length of file paths is
> defined by PATH_MAX with 4096 chars including nul.
> This limit exceeds MAX_BPF_STACK.
>
> Changelog:
>
> v3:
>  - Rebase
>
> v2:
>  - Add a test for bpf side
>
> Signed-off-by: Florian Lehner <dev@...-flo.net>
> ---

You dropped the ack from John, btw.

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

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

> +       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);
> +}

[...]