Re: [PATCH v16 06/12] bpf: Add bpf_lookup_*_key() and bpf_key_put() kfuncs

[Roberto Sassu <roberto.sassu@huaweicloud.com>]

On Tue, 2022-09-06 at 04:43 +0200, Kumar Kartikeya Dwivedi wrote:
> On Mon, 5 Sept 2022 at 16:35, Roberto Sassu
> <roberto.sassu@huaweicloud.com> wrote:
> > From: Roberto Sassu <roberto.sassu@huawei.com>
> > 
> > Add the bpf_lookup_user_key(), bpf_lookup_system_key() and
> > bpf_key_put()
> > kfuncs, to respectively search a key with a given key handle serial
> > number
> > and flags, obtain a key from a pre-determined ID defined in
> > include/linux/verification.h, and cleanup.
> > 
> > Introduce system_keyring_id_check() to validate the keyring ID
> > parameter of
> > bpf_lookup_system_key().
> > 
> > Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> > ---
> 
> With a small nit below,
> Acked-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
> 
> >  include/linux/bpf.h          |   8 +++
> >  include/linux/verification.h |   8 +++
> >  kernel/trace/bpf_trace.c     | 135
> > +++++++++++++++++++++++++++++++++++
> >  3 files changed, 151 insertions(+)
> > 
> > diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> > index 9dbd7c3f8929..f3db614aece6 100644
> > --- a/include/linux/bpf.h
> > +++ b/include/linux/bpf.h
> > @@ -2595,4 +2595,12 @@ static inline void bpf_cgroup_atype_get(u32
> > attach_btf_id, int cgroup_atype) {}
> >  static inline void bpf_cgroup_atype_put(int cgroup_atype) {}
> >  #endif /* CONFIG_BPF_LSM */
> > 
> > +struct key;
> > +
> > +#ifdef CONFIG_KEYS
> > +struct bpf_key {
> > +       struct key *key;
> > +       bool has_ref;
> > +};
> > +#endif /* CONFIG_KEYS */
> >  #endif /* _LINUX_BPF_H */
> > diff --git a/include/linux/verification.h
> > b/include/linux/verification.h
> > index a655923335ae..f34e50ebcf60 100644
> > --- a/include/linux/verification.h
> > +++ b/include/linux/verification.h
> > @@ -17,6 +17,14 @@
> >  #define VERIFY_USE_SECONDARY_KEYRING ((struct key *)1UL)
> >  #define VERIFY_USE_PLATFORM_KEYRING  ((struct key *)2UL)
> > 
> > +static inline int system_keyring_id_check(u64 id)
> > +{
> > +       if (id > (unsigned long)VERIFY_USE_PLATFORM_KEYRING)
> > +               return -EINVAL;
> > +
> > +       return 0;
> > +}
> > +
> >  /*
> >   * The use to which an asymmetric key is being put.
> >   */
> > diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
> > index 68e5cdd24cef..7a7023704ac2 100644
> > --- a/kernel/trace/bpf_trace.c
> > +++ b/kernel/trace/bpf_trace.c
> > @@ -20,6 +20,8 @@
> >  #include <linux/fprobe.h>
> >  #include <linux/bsearch.h>
> >  #include <linux/sort.h>
> > +#include <linux/key.h>
> > +#include <linux/verification.h>
> > 
> >  #include <net/bpf_sk_storage.h>
> > 
> > @@ -1181,6 +1183,139 @@ static const struct bpf_func_proto
> > bpf_get_func_arg_cnt_proto = {
> >         .arg1_type      = ARG_PTR_TO_CTX,
> >  };
> > 
> > +#ifdef CONFIG_KEYS
> > +__diag_push();
> > +__diag_ignore_all("-Wmissing-prototypes",
> > +                 "kfuncs which will be used in BPF programs");
> > +
> > +/**
> > + * bpf_lookup_user_key - lookup a key by its serial
> > + * @serial: key handle serial number
> > + * @flags: lookup-specific flags
> > + *
> > + * Search a key with a given *serial* and the provided *flags*.
> > + * If found, increment the reference count of the key by one, and
> > + * return it in the bpf_key structure.
> > + *
> > + * The bpf_key structure must be passed to bpf_key_put() when done
> > + * with it, so that the key reference count is decremented and the
> > + * bpf_key structure is freed.
> > + *
> > + * Permission checks are deferred to the time the key is used by
> > + * one of the available key-specific kfuncs.
> > + *
> > + * Set *flags* with KEY_LOOKUP_CREATE, to attempt creating a
> > requested
> > + * special keyring (e.g. session keyring), if it doesn't yet
> > exist.
> > + * Set *flags* with KEY_LOOKUP_PARTIAL, to lookup a key without
> > waiting
> > + * for the key construction, and to retrieve uninstantiated keys
> > (keys
> > + * without data attached to them).
> > + *
> > + * Return: a bpf_key pointer with a valid key pointer if the key
> > is found, a
> > + *         NULL pointer otherwise.
> > + */
> > +struct bpf_key *bpf_lookup_user_key(u32 serial, u64 flags)
> > +{
> > +       key_ref_t key_ref;
> > +       struct bpf_key *bkey;
> > +
> > +       if (flags & ~KEY_LOOKUP_ALL)
> > +               return NULL;
> > +
> > +       /*
> > +        * Permission check is deferred until the key is used, as
> > the
> > +        * intent of the caller is unknown here.
> > +        */
> > +       key_ref = lookup_user_key(serial, flags,
> > KEY_DEFER_PERM_CHECK);
> > +       if (IS_ERR(key_ref))
> > +               return NULL;
> > +
> > +       bkey = kmalloc(sizeof(*bkey), GFP_ATOMIC);
> 
> Since this function (due to lookup_user_key) is sleepable, do we
> really need GFP_ATOMIC here?

Daniel suggested it for bpf_lookup_system_key(), so that the kfunc does
not have to be sleepable. For symmetry, I did the same to
bpf_lookup_user_key(). Will switch back to GFP_KERNEL.

Thanks

Roberto