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:	Wed, 23 Jul 2014 13:25:16 -0700
From:	Kees Cook <>
To:	Alexei Starovoitov <>
Cc:	"David S. Miller" <>,
	Ingo Molnar <>,
	Linus Torvalds <>,
	Andy Lutomirski <>,
	Steven Rostedt <>,
	Daniel Borkmann <>,
	Chema Gonzalez <>,
	Eric Dumazet <>,
	Peter Zijlstra <>,
	Arnaldo Carvalho de Melo <>,
	Jiri Olsa <>,
	Thomas Gleixner <>,
	"H. Peter Anvin" <>,
	Andrew Morton <>,
	Linux API <>,
	Network Development <>,
	LKML <>
Subject: Re: [PATCH RFC v2 net-next 07/16] bpf: add lookup/update/delete/iterate
 methods to BPF maps

On Wed, Jul 23, 2014 at 12:49 PM, Alexei Starovoitov <> wrote:
> On Wed, Jul 23, 2014 at 11:25 AM, Kees Cook <> wrote:
>>> +
>>> +       /* lookup key in a given map referenced by map_id
>>> +        * err = bpf_map_lookup_elem(int map_id, void *key, void *value)
>> This needs map_id documentation updates too?
> yes. will grep for it just to make sure.
>>> +static int get_map_id(struct fd f)
>>> +{
>>> +       struct bpf_map *map;
>>> +
>>> +       if (!f.file)
>>> +               return -EBADF;
>>> +
>>> +       if (f.file->f_op != &bpf_map_fops) {
>>> +               fdput(f);
>> It feels weird to me to do the fdput inside this function. Instead,
>> should map_lookup_elem get a "err_put" label, instead?
> I don't think it will work, since I'm not sure that fd.flags will be zero
> when fd.file == NULL. It looks so by analyzing return code path
> in fs/file.c, but I wasn't sure that I followed all code paths,
> so I just picked this style from fs/timerfd.c assuming it was
> done this away on purpose and there can be the case where
> fd.file == null and fd.flags !=0. In such case we cannot call fdput().

Yeah, hm, looking around, this does seem to be the case. I guess the
thought is that when get_map_id fails, struct fd has been handled.
Maybe add a comment above that function as a reminder?

>>> +       err = -EFAULT;
>>> +       if (copy_to_user(uvalue, value, map->value_size) != 0)
>>> +               goto free_key;
>> I'm made uncomfortable with memory copying where explicit lengths from
>> userspace aren't being used. It does look like it would be redundant,
>> though. Are there other syscalls where the kernel may stomp on user
>> memory based on internal kernel sizes? I think this is fine as-is, but
>> it makes me want to think harder about it. :)
> good question :)
> key_size and value_size are passed initially from user space.
> Kernel only verifies and allocates internal map elements with given
> sizes. Then it copies the value back with the size it remembered.
> If user space said at map creation time that value_size is 100,
> it should be using it consistently in user space program.

Yeah, I think this should be fine as-is.

>>> +       err = -ENOMEM;
>>> +       next_key = kmalloc(map->key_size, GFP_ATOMIC);
>> In the interests of defensiveness, I'd use kzalloc here.
> I think it would be an overkill. Map implementation must consume
> all bytes of incoming 'key' and return exactly the same number
> of bytes in 'next_key'. Otherwise the whole iteration over map
> with 'get_next_key' won't work. So if map implementation is
> broken, it will be seen right away. No security leak here :)

Okay, fair enough. I had a few similar suggestions later. I kind of
wish there was a kcalloc that didn't zero memory to handle the case of
multiplied size input, but no need to spend the time clearing.

>>> +       case BPF_MAP_GET_NEXT_KEY:
>>> +               return map_get_next_key((int) arg2, (void __user *) arg3,
>>> +                                       (void __user *) arg4);
>> Same observation as the other syscall cmd: perhaps arg5 == 0 should be
>> checked? Also, since each of these functions looks up the fd and
> yes. will do.
>> builds the key, maybe those should be added to a common helper instead
>> of copy/pasting into each demuxed function?
> well, get_map_id() is a common helper. I didn't move fdget() all
> the way to switch statement, since it looks less readable.


Kees Cook
Chrome OS Security
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to
More majordomo info at

Powered by blists - more mailing lists