netdev - Re: [PATCH bpf-next] bpf: enable btf for use in all maps

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]

Message-ID: <20180809211416.oznmx5jlnbagkk3w@ast-mbp>
Date:   Thu, 9 Aug 2018 14:14:18 -0700
From:   Alexei Starovoitov <alexei.starovoitov@...il.com>
To:     Daniel Borkmann <daniel@...earbox.net>
Cc:     ast@...nel.org, netdev@...r.kernel.org, yhs@...com
Subject: Re: [PATCH bpf-next] bpf: enable btf for use in all maps

On Thu, Aug 09, 2018 at 09:42:20PM +0200, Daniel Borkmann wrote:
> Commit a26ca7c982cb ("bpf: btf: Add pretty print support to
> the basic arraymap") enabled support for BTF and dumping via
> BPF fs for arraymap. However, both can be decoupled from each
> other such that all BPF maps can be supported for attaching
> BTF key/value information, while not all maps necessarily
> need to dump via map_seq_show_elem() callback.
> 
> The check in array_map_check_btf() can be generalized as
> ultimatively the key and value size is the only contraint
> that needs to match for the map. The fact that the key needs
> to be of type int is optional; it could be any data type as
> long as it matches the 4 byte key size, just like hash table
> key or others could be of any data type as well.
> 
> Minimal example of a hash table dump which then works out
> of the box for bpftool:
> 
>   # bpftool map dump id 19
>   [{
>           "key": {
>               "": {
>                   "vip": 0,
>                   "vipv6": []
>               },
>               "port": 0,
>               "family": 0,
>               "proto": 0
>           },
>           "value": {
>               "flags": 0,
>               "vip_num": 0
>           }
>       }
>   ]
> 
> Signed-off-by: Daniel Borkmann <daniel@...earbox.net>
> Cc: Yonghong Song <yhs@...com>
> ---
>  include/linux/bpf.h   |  4 +---
>  kernel/bpf/arraymap.c | 27 ---------------------------
>  kernel/bpf/inode.c    |  3 ++-
>  kernel/bpf/syscall.c  | 24 ++++++++++++++++++++----
>  4 files changed, 23 insertions(+), 35 deletions(-)
> 
> diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> index cd8790d..eb76e8e 100644
> --- a/include/linux/bpf.h
> +++ b/include/linux/bpf.h
> @@ -48,8 +48,6 @@ struct bpf_map_ops {
>  	u32 (*map_fd_sys_lookup_elem)(void *ptr);
>  	void (*map_seq_show_elem)(struct bpf_map *map, void *key,
>  				  struct seq_file *m);
> -	int (*map_check_btf)(const struct bpf_map *map, const struct btf *btf,
> -			     u32 key_type_id, u32 value_type_id);
>  };
>  
>  struct bpf_map {
> @@ -118,7 +116,7 @@ static inline bool bpf_map_offload_neutral(const struct bpf_map *map)
>  
>  static inline bool bpf_map_support_seq_show(const struct bpf_map *map)
>  {
> -	return map->ops->map_seq_show_elem && map->ops->map_check_btf;
> +	return map->btf && map->ops->map_seq_show_elem;
>  }
>  
>  extern const struct bpf_map_ops bpf_map_offload_ops;
> diff --git a/kernel/bpf/arraymap.c b/kernel/bpf/arraymap.c
> index 2aa55d030..67f0bdf 100644
> --- a/kernel/bpf/arraymap.c
> +++ b/kernel/bpf/arraymap.c
> @@ -358,32 +358,6 @@ static void array_map_seq_show_elem(struct bpf_map *map, void *key,
>  	rcu_read_unlock();
>  }
>  
> -static int array_map_check_btf(const struct bpf_map *map, const struct btf *btf,
> -			       u32 btf_key_id, u32 btf_value_id)
> -{
> -	const struct btf_type *key_type, *value_type;
> -	u32 key_size, value_size;
> -	u32 int_data;
> -
> -	key_type = btf_type_id_size(btf, &btf_key_id, &key_size);
> -	if (!key_type || BTF_INFO_KIND(key_type->info) != BTF_KIND_INT)
> -		return -EINVAL;
> -
> -	int_data = *(u32 *)(key_type + 1);
> -	/* bpf array can only take a u32 key.  This check makes
> -	 * sure that the btf matches the attr used during map_create.
> -	 */
> -	if (BTF_INT_BITS(int_data) != 32 || key_size != 4 ||
> -	    BTF_INT_OFFSET(int_data))
> -		return -EINVAL;

I think most of these checks are still necessary for array type.
Relaxing BTF array key from BTF_KIND_INT to, for example, BTF_KIND_ENUM
is probably ok, but key being BTF_KIND_PTR or BTF_KIND_ARRAY doesn't makes sense.

For hash maps we probably need hash specific checks too. Otherwise
such sanity checks would need to be in kernel pretty printer and later
in user space too (bpftool and everything that will consume BTF),
since user space won't be able to trust kernel with sane key types.