Message-ID: <20180810021337.dmuwiyrygd3h2dbd@ast-mbp.dhcp.thefacebook.com>
Date:   Thu, 9 Aug 2018 19:13:38 -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 Fri, Aug 10, 2018 at 12:43:20AM +0200, Daniel Borkmann wrote:
> On 08/09/2018 11:44 PM, Alexei Starovoitov wrote:
> > On Thu, Aug 09, 2018 at 11:30:52PM +0200, Daniel Borkmann wrote:
> >> On 08/09/2018 11:14 PM, Alexei Starovoitov wrote:
> >>> 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.
> >>
> >> Hmm, so on 64 bit archs BTF_KIND_PTR would get rejected for array,
> >> on 32 bit it may be allowed due to sizeof(void *) == 4. BTF_KIND_ARRAY
> >> could be array of u8 foo[4], for example, or u16 foo[2]. But how would
> >> it ultimately be different from e.g. having 'struct a' versus 'struct b'
> >> where both are of same size and while actual key has 'struct a', the one
> >> who writes the prog resp. loads the BTF into the kernel would lie about
> >> it stating it's of type 'struct b' instead? It's basically trusting the
> >> app that it advertised sane key types which kernel is propagating back.
> > 
> > for hash map - yes. the kernel cannot yet catch the lie that
> > key == 'struct a' that user said in BTF is not what program used
> > (which used 'struct b' of the same size).
> > Eventually we will annotate all load/store in the program and will
> > make sure that memory access match what BTF said.
> 
> But in that case, would you reject the program? E.g. from prog point of
> view, it's just a buffer of x bytes, so key could be casted to different
> struct/types potentially and used for lookup; similar with value if you
> would go the route to annotate all access into it. I don't think this
> serves as a security feature (since you might as well just load the prog
> without BTF just fine), but it can be used to help verifier to perform
> rewrites like in tracing for implicit bpf_probe_read() based on member
> access. But also in that case, if you might have e.g. stale or wrong BTF
> data e.g. of the whole kernel or some application it would follow/walk
> that one instead. But such user error would be "acceptable" since it serves
> as a hint, roughly similar to (explicitly) walking the data structures
> based on the headers today, you do have better control in terms of header
> mismatches in that you can ship the BTF directly from the build, but there's
> still no guarantee in that sense that you "verified" that these bytes
> originally were mapped to struct foo somewhere in a C program.

I wouldn't view such key checks as safety feature, but rather
as honesty check. Meaning that user space shouldn't be able to cheat
by passing completely bogus BTF into the kernel that only
statisfies single size check.
Say, BTF has a key that is a 4 byte struct { char a,b,c,d; };
but program operates with it as u32. I very much would like
the verifier to notice and reject such program, since if C code
has struct { char a,b,c,d; }; the compiler won't generate u32 access
to it unless C code type casts, but then llvm will warn. So for u32
to legitimately appear in the generated code the struct should be:
union {
  struct { char a,b,c,d;}
  u32 e;
};
Narrow access (like u8 load/store in the bpf prog form u32 BTF field)
is ok, since that's normal compiler optimization, but any other
field/size mismatch the verifier should reject to prevent cheating.

In other words even proprietary bpf programs should not be able to cheat.
If they provide BTF to the kernel, it should correspond exactly to the program.
That's the key to _trusted_ introspection.
Admin that ssh-es into the box and operates with bpftool should be
certain that the map introspection represent the real situation of
the program. If proprietary prog is paranoid about layout of map
it shouldn't be using BTF, but if it does, BTF should match.
In the future I'd like to enfore availability of BTF for
new program types.

> > For array we can catch the lie today that key is not 4 byte int,
> > since it matters from pretty printing point of view.
> > If it's PTR or ARRAY or STRUCT, the printer will go nuts.
> 
> In that case, would you enforce a hash map key size of 4 also to INT-only
> instead of e.g. allowing STRUCT and various others?

hash map key of 4 bytes can be anything and printing is ideally
done similar to the way gdb prints stl c++:
#include <map>

struct K {
  int a, b;
};
struct V {
  int c, d;
};
int operator<(const K& a, const K& b) {return a.a < b.a;}
std::map<K, V> m;

int main()
{
  K k1 = {1, 2};
  K k2 = {3, 4};
  V v1 = {5, 6};
  V v2 = {7, 8};
  m[k1] = v1;
  m[k2] = v2;
...
(gdb) p m
$3 = std::map with 2 elements = {[{a = 1, b = 2}] = {c = 5, d = 6}, [{a = 3, b = 4}] = {c = 7, d = 8}}
(gdb) set print pretty on
(gdb) p m
$4 = std::map with 2 elements = {
  [{
    a = 1,
    b = 2
  }] = {
    c = 5,
    d = 6
  },
  [{
    a = 3,
    b = 4
  }] = {
    c = 7,
    d = 8
  }
}

Powered by blists - more mailing lists