Message-ID: <565C549C.5080408@iogearbox.net>
Date: Mon, 30 Nov 2015 14:52:28 +0100
From: Daniel Borkmann <daniel@iogearbox.net>
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.4.0
MIME-Version: 1.0
To: Alexei Starovoitov <alexei.starovoitov@gmail.com>, davem@davemloft.net
CC: Alexei Starovoitov <ast@kernel.org>, netdev <netdev@vger.kernel.org>,
        LKML <linux-kernel@vger.kernel.org>,
        syzkaller <syzkaller@googlegroups.com>,
        Kostya Serebryany <kcc@google.com>,
        Alexander Potapenko <glider@google.com>,
        Eric Dumazet <edumazet@google.com>,
        Sasha Levin <sasha.levin@oracle.com>
Subject: Re: [PATCH net] bpf: fix allocation warnings in bpf maps and integer
 overflow
References: <CACT4Y+b1k7Ofvn6xa=qWv9xP6BbteWOnZsJMXLZ=GOby-ajF=w@mail.gmail.com> <20151130005934.GA95228@ast-mbp.thefacebook.com>
In-Reply-To: <20151130005934.GA95228@ast-mbp.thefacebook.com>
Content-Type: text/plain; charset=windows-1252; format=flowed
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 2033
Lines: 56

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 linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/