Re: [PATCH v17 07/12] bpf: Add bpf_verify_pkcs7_signature() kfunc

[Kumar Kartikeya Dwivedi]

On Sun, 11 Sept 2022 at 13:41, KP Singh <kpsingh@xxxxxxxxxx> wrote:
>
> On Fri, Sep 9, 2022 at 2:09 PM Roberto Sassu
> <roberto.sassu@xxxxxxxxxxxxxxx> wrote:
> >
> > From: Roberto Sassu <roberto.sassu@xxxxxxxxxx>
> >
> > Add the bpf_verify_pkcs7_signature() kfunc, to give eBPF security modules
> > the ability to check the validity of a signature against supplied data, by
> > using user-provided or system-provided keys as trust anchor.
> >
> > The new kfunc makes it possible to enforce mandatory policies, as eBPF
> > programs might be allowed to make security decisions only based on data
> > sources the system administrator approves.
> >
> > The caller should provide the data to be verified and the signature as eBPF
> > dynamic pointers (to minimize the number of parameters) and a bpf_key
> > structure containing a reference to the keyring with keys trusted for
> > signature verification, obtained from bpf_lookup_user_key() or
> > bpf_lookup_system_key().
> >
> > For bpf_key structures obtained from the former lookup function,
> > bpf_verify_pkcs7_signature() completes the permission check deferred by
> > that function by calling key_validate(). key_task_permission() is already
> > called by the PKCS#7 code.
> >
> > Signed-off-by: Roberto Sassu <roberto.sassu@xxxxxxxxxx>
> > Acked-by: KP Singh <kpsingh@xxxxxxxxxx>
> > ---
> >  kernel/trace/bpf_trace.c | 45 ++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 45 insertions(+)
> >
> > diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
> > index ab183dbaa8d1..9df53c40cffd 100644
> > --- a/kernel/trace/bpf_trace.c
> > +++ b/kernel/trace/bpf_trace.c
> > @@ -1294,12 +1294,57 @@ void bpf_key_put(struct bpf_key *bkey)
> >         kfree(bkey);
> >  }
> >
> > +#ifdef CONFIG_SYSTEM_DATA_VERIFICATION
> > +/**
> > + * bpf_verify_pkcs7_signature - verify a PKCS#7 signature
> > + * @data_ptr: data to verify
> > + * @sig_ptr: signature of the data
> > + * @trusted_keyring: keyring with keys trusted for signature verification
> > + *
> > + * Verify the PKCS#7 signature *sig_ptr* against the supplied *data_ptr*
> > + * with keys in a keyring referenced by *trusted_keyring*.
> > + *
> > + * Return: 0 on success, a negative value on error.
> > + */
> > +int bpf_verify_pkcs7_signature(struct bpf_dynptr_kern *data_ptr,
> > +                              struct bpf_dynptr_kern *sig_ptr,
> > +                              struct bpf_key *trusted_keyring)
> > +{
> > +       int ret;
> > +
> > +       if (trusted_keyring->has_ref) {
> > +               /*
> > +                * Do the permission check deferred in bpf_lookup_user_key().
> > +                * See bpf_lookup_user_key() for more details.
> > +                *
> > +                * A call to key_task_permission() here would be redundant, as
> > +                * it is already done by keyring_search() called by
> > +                * find_asymmetric_key().
> > +                */
> > +               ret = key_validate(trusted_keyring->key);
> > +               if (ret < 0)
> > +                       return ret;
> > +       }
> > +
> > +       return verify_pkcs7_signature(data_ptr->data,
> > +                                     bpf_dynptr_get_size(data_ptr),
> > +                                     sig_ptr->data,
> > +                                     bpf_dynptr_get_size(sig_ptr),
> > +                                     trusted_keyring->key,
> > +                                     VERIFYING_UNSPECIFIED_SIGNATURE, NULL,
> > +                                     NULL);
> > +}
>
> This seems to work if the data that needs to be verified
> and the signature is allocated onto the map.
>
> For BPF program signing, the signature will be void * pointer (and length)
> in a struct in the kernel
>
> +++ b/include/uapi/linux/bpf.h
> @@ -1383,6 +1383,8 @@ union bpf_attr {
>                 __aligned_u64   fd_array;       /* array of FDs */
>                 __aligned_u64   core_relos;
>                 __u32           core_relo_rec_size; /* sizeof(struct
> bpf_core_relo) */
> +               __aligned_u64   signature;
> +               __u32           signature_size;
>         };
>
> Something like this in the bpf_prog_aux struct which is passed to
> security_bpf_prog_alloc.
>
> Now creating a dynptr to use with this kfunc does not work:
>
>    bpf_dynptr_from_mem(aux->signature, aux->signature_size, 0, &sig_ptr);
>
> So one has to copy kernel data into a map and then create dynptrs.
> Would you be able to update
> the dynptr logic to handle this case too? (follow up is okay too).
>

ISTM it needs the feature first before it can be added.
To make it work like map_val, value_size(which is a constant) to pass
to bpf_dynptr_from_mem,
verifier will have to mark load of aux->signature as PTR_TO_MEM with the known
constant size, and then mark_reg_known for scalar reg for aux->signature_size.
Since we need to know that 0 <= r2 <= r1.mem_size.
This would require some work on the btf_struct_access handling.

It cannot be made to work in the general case of void * and len.
There might also be other better options (like kernel itself preparing
read only bpf_dynptr struct in bpf_prog_aux for the signature) so you
can pass its address directly to the kfunc.