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

[Daniel Borkmann]

On 11/30/2015 07:13 PM, Alexei Starovoitov wrote:
> On Mon, Nov 30, 2015 at 03:34:35PM +0100, Daniel Borkmann wrote:
> > > > 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 ...
>
> not quite, the above check is for kmalloc-s in syscall.c
>
> > Ok, I see. The check and comment is related to the fact that when we do bpf(2)
> > syscall to lookup an element:
> >
> > We call map_lookup_elem(), which does kmalloc() on the value_size.
> >
> > So an individual entry lookup could fail with kmalloc() there, unrelated to an
> > individual map implementation.
>
> kmalloc with order >= MAX_ORDER warning can be seen in syscall for update/lookup
> commands regardless of map implememtation.
> So the maps with "value_size >= 1 << (KMALLOC_SHIFT_MAX - 1)" were not accessible
> from user space anyway.
> This check in arraymap.c fixes the warning and prevents creation of such
> maps in the first place as the comment right below it says.

Yeah, right. Noticed that later on. It was a bit confusing at first as I didn't
parse that clearly from the commit message itself.

> Similar check in hashmap.c fixes warning, prevents abnormal map creation and fixes
> integer overflow which is the most dangerous of them all.
>
> The check in arraymap.c
> -        attr->max_entries > (U32_MAX - sizeof(*array)) / elem_size)
> +        attr->max_entries > (U32_MAX - PAGE_SIZE - sizeof(*array)) / elem_size)
>   fixes potential integer overflow in map.pages computation.
>
> and similar check in hashtab.c:
> (u64) htab->elem_size * htab->map.max_entries >= U32_MAX - PAGE_SIZE
> fixes integer overflow in map.pages as well.

Yep, got that part.

> the 'value_size >= (1 << (KMALLOC_SHIFT_MAX - 1)) - MAX_BPF_STACK - sizeof(struct htab_elem)'
> check in hashmap.c fixes integer overflow in elem_size and
> makes elem_size kmalloc-able later in htab_map_update_elem().
> Since it wasn't obvious that this one 'if' addresses these multiple issues,
> I've added a comment there.

... and the MAX_BPF_STACK stands for the maximum key part here, okay.

So, when creating a sufficiently large map where map->key_size + map->value_size
would be > MAX_BPF_STACK (but map->key_size still <= MAX_BPF_STACK), we can only
read the map from an eBPF program, but not update it. In such cases, updates could
only happen from user space application.

> Addition of __GFP_NOWARN only fixes OOM warning as commit log says.

That's obvious, too.

> > Hmm, seems this patch fixes many things at once, maybe makes sense to split it?
>
> hmm I don't see a point of changing the same single line over multipe patches.
> The split won't help backporting, but rather makes for more patches to deal with.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/