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

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]

Date:	Mon, 30 Nov 2015 14:52:28 +0100
From:	Daniel Borkmann <daniel@...earbox.net>
To:	Alexei Starovoitov <alexei.starovoitov@...il.com>,
	davem@...emloft.net
CC:	Alexei Starovoitov <ast@...nel.org>,
	netdev <netdev@...r.kernel.org>,
	LKML <linux-kernel@...r.kernel.org>,
	syzkaller <syzkaller@...glegroups.com>,
	Kostya Serebryany <kcc@...gle.com>,
	Alexander Potapenko <glider@...gle.com>,
	Eric Dumazet <edumazet@...gle.com>,
	Sasha Levin <sasha.levin@...cle.com>
Subject: Re: [PATCH net] bpf: fix allocation warnings in bpf maps and integer
 overflow

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
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html