Re: [PATCH net] bpf: fix allocation warnings in bpf maps and integer overflow

[Daniel Borkmann <daniel@iogearbox.net>]

On 11/30/2015 01:59 AM, Alexei Starovoitov wrote:
[...]
> For large map->value_size the user space can trigger memory allocation warnings like:
[...]

> To avoid never succeeding kmalloc with order >= MAX_ORDER check that
> elem->value_size and computed elem_size are within limits for both hash and
> array type maps.
[...]

> Large value_size can cause integer overflows in elem_size and map.pages
> formulas, so check for that as well.
[...]

> diff --git a/kernel/bpf/arraymap.c b/kernel/bpf/arraymap.c
> index 3f4c99e06c6b..b1e53b79c586 100644
> --- a/kernel/bpf/arraymap.c
> +++ b/kernel/bpf/arraymap.c
> @@ -28,11 +28,17 @@ static struct bpf_map *array_map_alloc(union bpf_attr *attr)
>   	    attr->value_size == 0)
>   		return ERR_PTR(-EINVAL);
>
> +	if (attr->value_size >= 1 << (KMALLOC_SHIFT_MAX - 1))
> +		/* if value_size is bigger, the user space won't be able to
> +		 * access the elements.
> +		 */
> +		return ERR_PTR(-E2BIG);
> +

Bit confused, given that in array map, we try kzalloc() with __GFP_NOWARN already
and if that fails, we fall back to vzalloc(), it shouldn't trigger memory allocation
warnings here ...

Then, integer overflow in elem_size with round_up(attr->value_size, 8) could only
result in 0, which is already tested below.

>   	elem_size = round_up(attr->value_size, 8);
>
>   	/* check round_up into zero and u32 overflow */
>   	if (elem_size == 0 ||
> -	    attr->max_entries > (U32_MAX - sizeof(*array)) / elem_size)
> +	    attr->max_entries > (U32_MAX - PAGE_SIZE - sizeof(*array)) / elem_size)
>   		return ERR_PTR(-ENOMEM);

... and this change seems to be needed for the integer overflow in map.pages?

So if the first check above intends to check for some size overflow (?), how is it
then related to KMALLOC_SHIFT_MAX?

Thanks,
Daniel