netdev - Re: [PATCH bpf-next v4] 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: <CAEf4BzZbtdgK-6y1cX6U2_sV9T6QHO=fAj2j0L_CtuqW0DZ1Rw@mail.gmail.com>
Date:   Wed, 28 Oct 2020 19:36:26 -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 v4] bpf: Lift hashtab key_size limit

On Wed, Oct 28, 2020 at 3:21 PM 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:
>
> v4:
>  - Utilize BPF skeleton in tests
>  - Rebase
>
> v3:
>  - Rebase
>
> v2:
>  - Add a test for bpf side
>
> Signed-off-by: Florian Lehner <dev@...-flo.net>
> Acked-by: John Fastabend <john.fastabend@...il.com>
> ---
>  kernel/bpf/hashtab.c                          | 16 ++-----
>  .../selftests/bpf/prog_tests/hash_large_key.c | 43 +++++++++++++++++
>  .../selftests/bpf/progs/test_hash_large_key.c | 46 +++++++++++++++++++
>  tools/testing/selftests/bpf/test_maps.c       |  3 +-
>  4 files changed, 96 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..fff7cd05b9e3 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 ((u64)(attr->key_size + attr->value_size) >= KMALLOC_MAX_SIZE -

this will add both as u32, then will cast overflown result to u64.
Instead just do:

if ((u64)attr->key_size + attr->value_size >= ....)

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

[...]

> +
> +SEC("raw_tracepoint/sys_enter")
> +int bpf_hash_large_key_test(void *ctx)
> +{
> +       int zero = 0, err = 1, value = 42;
> +       struct bigelement *key;
> +
> +       key = bpf_map_lookup_elem(&key_map, &zero);
> +       if (!key)
> +               goto err;
> +
> +       key->c = 1;
> +       if (bpf_map_update_elem(&hash_map, key, &value, BPF_ANY))
> +               goto err;
> +
> +       err = 0;
> +err:
> +       return err;

return value from raw_tracepoint doesn't really communicate error, you
might as well just return 0 always

> +}
> +
> diff --git a/tools/testing/selftests/bpf/test_maps.c b/tools/testing/selftests/bpf/test_maps.c
> index 0d92ebcb335d..0ad3e6305ff0 100644
> --- a/tools/testing/selftests/bpf/test_maps.c
> +++ b/tools/testing/selftests/bpf/test_maps.c
> @@ -1223,9 +1223,10 @@ static void test_map_in_map(void)
>
>  static void test_map_large(void)
>  {
> +
>         struct bigkey {
>                 int a;
> -               char b[116];
> +               char b[4096];
>                 long long c;
>         } key;
>         int fd, i, value;
> --
> 2.26.2
>