[V2,bpf,1/2] bpf: Check bloom filter map value size

Commit Message

Andrei Matei March 27, 2024, 2:42 a.m. UTC

This patch adds a missing check to bloom filter creating, rejecting
values above KMALLOC_MAX_SIZE. This brings the bloom map in line with
many other map types.

The lack of this protection can cause kernel crashes for value sizes
that overflow int's. Such a crash was caught by syzkaller. The next
patch adds more guard-rails at a lower level.

Signed-off-by: Andrei Matei <[e-mail]>
---
 kernel/bpf/bloom_filter.c                           | 13 +++++++++++++
 .../selftests/bpf/prog_tests/bloom_filter_map.c     |  6 ++++++
 2 files changed, 19 insertions(+)

Comments

Andrii Nakryiko March 27, 2024, 4:48 p.m. UTC | #1

On Tue, Mar 26, 2024 at 7:43 PM Andrei Matei <[e-mail]> wrote:
>
> This patch adds a missing check to bloom filter creating, rejecting
> values above KMALLOC_MAX_SIZE. This brings the bloom map in line with
> many other map types.
>
> The lack of this protection can cause kernel crashes for value sizes
> that overflow int's. Such a crash was caught by syzkaller. The next
> patch adds more guard-rails at a lower level.
>
> Signed-off-by: Andrei Matei <[e-mail]>
> ---
>  kernel/bpf/bloom_filter.c                           | 13 +++++++++++++
>  .../selftests/bpf/prog_tests/bloom_filter_map.c     |  6 ++++++
>  2 files changed, 19 insertions(+)
>

Acked-by: Andrii Nakryiko <[e-mail]>

> diff --git a/kernel/bpf/bloom_filter.c b/kernel/bpf/bloom_filter.c
> index addf3dd57b59..35e1ddca74d2 100644
> --- a/kernel/bpf/bloom_filter.c
> +++ b/kernel/bpf/bloom_filter.c
> @@ -80,6 +80,18 @@ static int bloom_map_get_next_key(struct bpf_map *map, void *key, void *next_key
>         return -EOPNOTSUPP;
>  }
>
> +/* Called from syscall */
> +static int bloom_map_alloc_check(union bpf_attr *attr)
> +{
> +       if (attr->value_size > KMALLOC_MAX_SIZE)
> +               /* if value_size is bigger, the user space won't be able to
> +                * access the elements.
> +                */
> +               return -E2BIG;
> +
> +       return 0;
> +}
> +
>  static struct bpf_map *bloom_map_alloc(union bpf_attr *attr)
>  {
>         u32 bitset_bytes, bitset_mask, nr_hash_funcs, nr_bits;
> @@ -191,6 +203,7 @@ static u64 bloom_map_mem_usage(const struct bpf_map *map)
>  BTF_ID_LIST_SINGLE(bpf_bloom_map_btf_ids, struct, bpf_bloom_filter)
>  const struct bpf_map_ops bloom_filter_map_ops = {
>         .map_meta_equal = bpf_map_meta_equal,
> +       .map_alloc_check = bloom_map_alloc_check,
>         .map_alloc = bloom_map_alloc,
>         .map_free = bloom_map_free,
>         .map_get_next_key = bloom_map_get_next_key,
> diff --git a/tools/testing/selftests/bpf/prog_tests/bloom_filter_map.c b/tools/testing/selftests/bpf/prog_tests/bloom_filter_map.c
> index 053f4d6da77a..cc184e4420f6 100644
> --- a/tools/testing/selftests/bpf/prog_tests/bloom_filter_map.c
> +++ b/tools/testing/selftests/bpf/prog_tests/bloom_filter_map.c
> @@ -2,6 +2,7 @@
>  /* Copyright (c) 2021 Facebook */
>
>  #include <sys/syscall.h>
> +#include <limits.h>
>  #include <test_progs.h>
>  #include "bloom_filter_map.skel.h"
>
> @@ -21,6 +22,11 @@ static void test_fail_cases(void)
>         if (!ASSERT_LT(fd, 0, "bpf_map_create bloom filter invalid value size 0"))
>                 close(fd);
>
> +       /* Invalid value size: too big */
> +       fd = bpf_map_create(BPF_MAP_TYPE_BLOOM_FILTER, NULL, 0, INT32_MAX, 100, NULL);
> +       if (!ASSERT_LT(fd, 0, "bpf_map_create bloom filter invalid value too large"))
> +               close(fd);
> +
>         /* Invalid max entries size */
>         fd = bpf_map_create(BPF_MAP_TYPE_BLOOM_FILTER, NULL, 0, sizeof(value), 0, NULL);
>         if (!ASSERT_LT(fd, 0, "bpf_map_create bloom filter invalid max entries size"))
> --
> 2.40.1
>
>

Patch

diff --git a/kernel/bpf/bloom_filter.c b/kernel/bpf/bloom_filter.c
index addf3dd57b59..35e1ddca74d2 100644
--- a/kernel/bpf/bloom_filter.c
+++ b/kernel/bpf/bloom_filter.c
@@ -80,6 +80,18 @@  static int bloom_map_get_next_key(struct bpf_map *map, void *key, void *next_key
 	return -EOPNOTSUPP;
 }
 
+/* Called from syscall */
+static int bloom_map_alloc_check(union bpf_attr *attr)
+{
+	if (attr->value_size > KMALLOC_MAX_SIZE)
+		/* if value_size is bigger, the user space won't be able to
+		 * access the elements.
+		 */
+		return -E2BIG;
+
+	return 0;
+}
+
 static struct bpf_map *bloom_map_alloc(union bpf_attr *attr)
 {
 	u32 bitset_bytes, bitset_mask, nr_hash_funcs, nr_bits;
@@ -191,6 +203,7 @@  static u64 bloom_map_mem_usage(const struct bpf_map *map)
 BTF_ID_LIST_SINGLE(bpf_bloom_map_btf_ids, struct, bpf_bloom_filter)
 const struct bpf_map_ops bloom_filter_map_ops = {
 	.map_meta_equal = bpf_map_meta_equal,
+	.map_alloc_check = bloom_map_alloc_check,
 	.map_alloc = bloom_map_alloc,
 	.map_free = bloom_map_free,
 	.map_get_next_key = bloom_map_get_next_key,
diff --git a/tools/testing/selftests/bpf/prog_tests/bloom_filter_map.c b/tools/testing/selftests/bpf/prog_tests/bloom_filter_map.c
index 053f4d6da77a..cc184e4420f6 100644
--- a/tools/testing/selftests/bpf/prog_tests/bloom_filter_map.c
+++ b/tools/testing/selftests/bpf/prog_tests/bloom_filter_map.c
@@ -2,6 +2,7 @@ 
 /* Copyright (c) 2021 Facebook */
 
 #include <sys/syscall.h>
+#include <limits.h>
 #include <test_progs.h>
 #include "bloom_filter_map.skel.h"
 
@@ -21,6 +22,11 @@  static void test_fail_cases(void)
 	if (!ASSERT_LT(fd, 0, "bpf_map_create bloom filter invalid value size 0"))
 		close(fd);
 
+	/* Invalid value size: too big */
+	fd = bpf_map_create(BPF_MAP_TYPE_BLOOM_FILTER, NULL, 0, INT32_MAX, 100, NULL);
+	if (!ASSERT_LT(fd, 0, "bpf_map_create bloom filter invalid value too large"))
+		close(fd);
+
 	/* Invalid max entries size */
 	fd = bpf_map_create(BPF_MAP_TYPE_BLOOM_FILTER, NULL, 0, sizeof(value), 0, NULL);
 	if (!ASSERT_LT(fd, 0, "bpf_map_create bloom filter invalid max entries size"))