[PATCH bpf-next] bpf: check max_entries before allocating memory

[PATCH bpf-next] bpf: check max_entries before allocating memory

[Florian Lehner]

For maps of type BPF_MAP_TYPE_CPUMAP memory is allocated first before
checking the max_entries argument. If then max_entries is greater than
NR_CPUS additional work needs to be done to free allocated memory before
an error is returned.
This changes moves the check on max_entries before the allocation
happens.

Signed-off-by: Florian Lehner <dev@der-flo.net>
---
 kernel/bpf/cpumap.c | 19 ++++++++-----------
 1 file changed, 8 insertions(+), 11 deletions(-)

diff --git a/kernel/bpf/cpumap.c b/kernel/bpf/cpumap.c
index b5ba34ddd4b6..87e9f89a8140 100644
--- a/kernel/bpf/cpumap.c
+++ b/kernel/bpf/cpumap.c
@@ -97,29 +97,26 @@ static struct bpf_map *cpu_map_alloc(union bpf_attr *attr)
 	    attr->map_flags & ~BPF_F_NUMA_NODE)
 		return ERR_PTR(-EINVAL);
 
+	/* Pre-limit array size based on NR_CPUS, not final CPU check */
+	if (attr->max_entries > NR_CPUS)
+		return ERR_PTR(-E2BIG);
+
 	cmap = bpf_map_area_alloc(sizeof(*cmap), NUMA_NO_NODE);
 	if (!cmap)
 		return ERR_PTR(-ENOMEM);
 
 	bpf_map_init_from_attr(&cmap->map, attr);
 
-	/* Pre-limit array size based on NR_CPUS, not final CPU check */
-	if (cmap->map.max_entries > NR_CPUS) {
-		err = -E2BIG;
-		goto free_cmap;
-	}
-
 	/* Alloc array for possible remote "destination" CPUs */
 	cmap->cpu_map = bpf_map_area_alloc(cmap->map.max_entries *
 					   sizeof(struct bpf_cpu_map_entry *),
 					   cmap->map.numa_node);
-	if (!cmap->cpu_map)
-		goto free_cmap;
+	if (!cmap->cpu_map) {
+		bpf_map_area_free(cmap);
+		return ERR_PTR(err);
+	}
 
 	return &cmap->map;
-free_cmap:
-	bpf_map_area_free(cmap);
-	return ERR_PTR(err);
 }
 
 static void get_cpu_map_entry(struct bpf_cpu_map_entry *rcpu)
-- 
2.37.3

Re: [PATCH bpf-next] bpf: check max_entries before allocating memory

[Florian Lehner]

On Wed, Oct 26, 2022 at 10:50:53AM +0200, Florian Lehner wrote:
> For maps of type BPF_MAP_TYPE_CPUMAP memory is allocated first before
> checking the max_entries argument. If then max_entries is greater than
> NR_CPUS additional work needs to be done to free allocated memory before
> an error is returned.
> This changes moves the check on max_entries before the allocation
> happens.
> 
> Signed-off-by: Florian Lehner <dev@der-flo.net>

I did have a look at the failing CI tests:

Running bpftool checks...
  Comparing /tmp/work/bpf/bpf/tools/include/uapi/linux/bpf.h (bpf_map_type)
  and /tmp/work/bpf/bpf/tools/bpf/bpftool/map.c (do_help() TYPE):
  {'cgroup_storage', 'cgroup_storage_deprecated'}
  Comparing /tmp/work/bpf/bpf/tools/include/uapi/linux/bpf.h (bpf_map_type)
  and /tmp/work/bpf/bpf/tools/bpf/bpftool/Documentation/bpftool-map.rst (TYPE):
  {'cgroup_storage', 'cgroup_storage_deprecated'}
  bpftool checks returned 1.

It looks to me these were introduced with commit
a48b4bf994296a380f9c79620ad4ee7bad4511e1 and are not related to this
proposed change.

Re: [PATCH bpf-next] bpf: check max_entries before allocating memory

[Martin KaFai Lau]

On 10/26/22 1:50 AM, Florian Lehner wrote:
> For maps of type BPF_MAP_TYPE_CPUMAP memory is allocated first before
> checking the max_entries argument. If then max_entries is greater than
> NR_CPUS additional work needs to be done to free allocated memory before
> an error is returned.
> This changes moves the check on max_entries before the allocation
> happens.
> 
> Signed-off-by: Florian Lehner <dev@der-flo.net>
> ---
>   kernel/bpf/cpumap.c | 19 ++++++++-----------
>   1 file changed, 8 insertions(+), 11 deletions(-)
> 
> diff --git a/kernel/bpf/cpumap.c b/kernel/bpf/cpumap.c
> index b5ba34ddd4b6..87e9f89a8140 100644
> --- a/kernel/bpf/cpumap.c
> +++ b/kernel/bpf/cpumap.c
> @@ -97,29 +97,26 @@ static struct bpf_map *cpu_map_alloc(union bpf_attr *attr)
>   	    attr->map_flags & ~BPF_F_NUMA_NODE)
>   		return ERR_PTR(-EINVAL);
>   
> +	/* Pre-limit array size based on NR_CPUS, not final CPU check */
> +	if (attr->max_entries > NR_CPUS)
> +		return ERR_PTR(-E2BIG);
> +
>   	cmap = bpf_map_area_alloc(sizeof(*cmap), NUMA_NO_NODE);
>   	if (!cmap)
>   		return ERR_PTR(-ENOMEM);
>   
>   	bpf_map_init_from_attr(&cmap->map, attr);
>   
> -	/* Pre-limit array size based on NR_CPUS, not final CPU check */
> -	if (cmap->map.max_entries > NR_CPUS) {
> -		err = -E2BIG;
> -		goto free_cmap;
> -	}
> -
>   	/* Alloc array for possible remote "destination" CPUs */
>   	cmap->cpu_map = bpf_map_area_alloc(cmap->map.max_entries *
>   					   sizeof(struct bpf_cpu_map_entry *),
>   					   cmap->map.numa_node);
> -	if (!cmap->cpu_map)
> -		goto free_cmap;
> +	if (!cmap->cpu_map) {
> +		bpf_map_area_free(cmap);
> +		return ERR_PTR(err);

lgtm.  May as well take this chance to remove the "err" variable and directly 
return ERR_PTR(-ENOMEM) instead.

> +	}
>   
>   	return &cmap->map;
> -free_cmap:
> -	bpf_map_area_free(cmap);
> -	return ERR_PTR(err);
>   }
>   
>   static void get_cpu_map_entry(struct bpf_cpu_map_entry *rcpu)