netdev - Re: [PATCH 08/11] bpf: Add BTF whitelist support

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: <20200619132929.GI2465907@krava>
Date:   Fri, 19 Jun 2020 15:29:29 +0200
From:   Jiri Olsa <jolsa@...hat.com>
To:     Andrii Nakryiko <andrii.nakryiko@...il.com>
Cc:     Jiri Olsa <jolsa@...nel.org>, Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        Networking <netdev@...r.kernel.org>, bpf <bpf@...r.kernel.org>,
        Song Liu <songliubraving@...com>, Yonghong Song <yhs@...com>,
        Martin KaFai Lau <kafai@...com>,
        David Miller <davem@...hat.com>,
        John Fastabend <john.fastabend@...il.com>,
        Wenbo Zhang <ethercflow@...il.com>,
        KP Singh <kpsingh@...omium.org>,
        Andrii Nakryiko <andriin@...com>,
        Brendan Gregg <bgregg@...flix.com>,
        Florent Revest <revest@...omium.org>,
        Al Viro <viro@...iv.linux.org.uk>
Subject: Re: [PATCH 08/11] bpf: Add BTF whitelist support

On Thu, Jun 18, 2020 at 09:29:58PM -0700, Andrii Nakryiko wrote:

SNIP

> > @@ -4669,3 +4670,15 @@ u32 btf_id(const struct btf *btf)
> >  {
> >         return btf->id;
> >  }
> > +
> > +static int btf_id_cmp_func(const void *a, const void *b)
> > +{
> > +       const int *pa = a, *pb = b;
> > +
> > +       return *pa - *pb;
> > +}
> > +
> > +bool btf_whitelist_search(int id, int list[], int cnt)
> 
> whitelist is a bit too specific, this functionality can be used for
> blacklisting as well, no?
> 
> How about instead of "open coding" separately int list[] + int cnt, we
> define a struct:
> 
> struct btf_id_set {
>     u32 cnt;
>     u32 ids[];
> };
> 
> and pass that around?
> 
> This function then can be generic
> 
> bool btf_id_set_contains(struct btf_id_set *set, u32 id);
> 
> Then it's usable for both whitelist and blacklist? _contains also
> clearly implies what's the return result, while _search isn't so clear
> in that regard.

yep, looks better this way, will change

> 
> 
> > +{
> > +       return bsearch(&id, list, cnt, sizeof(int), btf_id_cmp_func) != NULL;
> > +}
> > diff --git a/kernel/bpf/btf_ids.h b/kernel/bpf/btf_ids.h
> > index 68aa5c38a37f..a90c09faa515 100644
> > --- a/kernel/bpf/btf_ids.h
> > +++ b/kernel/bpf/btf_ids.h
> > @@ -67,4 +67,42 @@ asm(                                                 \
> >  #name ":;                                      \n"     \
> >  ".popsection;                                  \n");
> >
> > +
> > +/*
> > + * The BTF_WHITELIST_ENTRY/END macros pair defines sorted
> > + * list of BTF IDs plus its members count, with following
> > + * layout:
> > + *
> > + * BTF_WHITELIST_ENTRY(list2)
> > + * BTF_ID(type1, name1)
> > + * BTF_ID(type2, name2)
> > + * BTF_WHITELIST_END(list)
> 
> It kind of sucks you need two separate ENTRY/END macro (btw, START/END
> or BEGIN/END would be a bit more "paired"), and your example clearly

ok, START/END it is

> shows why: it is not self-consistent (list2 on start, list on end ;).

ugh ;-)

> But doing variadic macro like this would be a nightmare as well,
> unfortunately. :(
> 
> > + *
> > + * __BTF_ID__sort__list:
> > + * list2_cnt:
> > + * .zero 4
> > + * list2:
> > + * __BTF_ID__type1__name1__3:
> > + * .zero 4
> > + * __BTF_ID__type2__name2__4:
> > + * .zero 4
> > + *
> > + */
> > +#define BTF_WHITELIST_ENTRY(name)                      \
> > +asm(                                                   \
> > +".pushsection " SECTION ",\"a\";               \n"     \
> > +".global __BTF_ID__sort__" #name ";            \n"     \
> > +"__BTF_ID__sort__" #name ":;                   \n"     \
> 
> I mentioned in the previous patch already, I think "sort" is a bad
> name, consider "set" (or "list", but you used list name already for a
> slightly different macro).

yes, I replied to this in another email

> 
> > +".global " #name "_cnt;                        \n"     \
> > +#name "_cnt:;                                  \n"     \
> 
> This label/symbol isn't necessary, why polluting the symbol table?

XXX_cnt variable is used in search function, but isn't needed
if we use that 'struct btf_id_set' you proposed

> 
> > +".zero 4                                       \n"     \
> > +".popsection;                                  \n");   \
> > +BTF_ID_LIST(name)
> > +
> > +#define BTF_WHITELIST_END(name)                                \
> > +asm(                                                   \
> > +".pushsection " SECTION ",\"a\";              \n"      \
> > +".size __BTF_ID__sort__" #name ", .-" #name " \n"      \
> > +".popsection;                                 \n");
> > +
> >  #endif
> > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> > index bee3da2cd945..5a9a6fd72907 100644
> > --- a/kernel/bpf/verifier.c
> > +++ b/kernel/bpf/verifier.c
> > @@ -4633,6 +4633,11 @@ static int check_helper_call(struct bpf_verifier_env *env, int func_id, int insn
> >                 return -EINVAL;
> >         }
> >
> > +       if (fn->allowed && !fn->allowed(env->prog)) {
> > +               verbose(env, "helper call is not allowed in probe\n");
> 
> nit: probe -> program, or just drop "in probe" part altogether

ok

thnaks,
jirka