Re: [PATCH RFC v2 net-next 08/16] bpf: add hashtable type of BPF maps

On Wed, Jul 23, 2014 at 12:57 PM, Alexei Starovoitov [off-list ref] wrote:

On Wed, Jul 23, 2014 at 11:36 AM, Kees Cook [off-list ref] wrote:

quoted

quoted

+static struct bpf_map *htab_map_alloc(struct nlattr *attr[BPF_MAP_ATTR_MAX + 1])
+{
+       struct bpf_htab *htab;
+       int err, i;
+
+       htab = kmalloc(sizeof(*htab), GFP_USER);

I'd prefer kzalloc here.

in this case I agree. will change, since it's not in critical path and we
can waste few cycles zeroing memory.

quoted

quoted

+       err = -ENOMEM;
+       htab->buckets = kmalloc(htab->n_buckets * sizeof(struct hlist_head),
+                               GFP_USER);

I'd prefer kcalloc here, even though n_buckets can't currently trigger
an integer overflow.

hmm, I would argue that kmalloc_array is a preferred way, but kcalloc ?
Few lines below the whole array is inited with INIT_HLIST_HEAD...

Ah! I didn't realize kmalloc_array existed! Perfect. Yes, that would
be great to use. The zeroing is not needed, due to the init below, as
you say.

quoted

quoted

+       for (i = 0; i < htab->n_buckets; i++)
+               INIT_HLIST_HEAD(&htab->buckets[i]);

quoted

quoted

+       htab->slab_name = kasprintf(GFP_USER, "bpf_htab_%p", htab);

This leaks a kernel heap memory pointer to userspace. If a unique name
needed, I think map_id should be used instead.

it leaks, how? slabinfo is only available to root.
The same code exists in conntrack:
net/netfilter/nf_conntrack_core.c:1767

Right, in extreme cases, there are system configurations where leaking
addresses even to root can be considered a bug. There are a lot of
these situations in the kernel still, that's true. However, if we can
at all avoid it, I'd really like to avoid adding new ones. Nearly all
the cases of using a memory pointer is for uniqueness concerns, but I
think can already get that from the map_id.

-Kees

-- 
Kees Cook
Chrome OS Security