lkml.org 
[lkml]   [2022]   [Jun]   [10]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
Date
SubjectRe: [PATCH v3 1/2] bpf: Add bpf_verify_signature() helper
On Fri, Jun 10, 2022 at 5:14 PM Daniel Borkmann <daniel@iogearbox.net> wrote:
>
> On 6/10/22 4:59 PM, Roberto Sassu wrote:
> >> From: Daniel Borkmann [mailto:daniel@iogearbox.net]
> >> Sent: Friday, June 10, 2022 4:49 PM
> >> On 6/10/22 3:59 PM, Roberto Sassu wrote:
> >>> Add the bpf_verify_signature() helper, to give eBPF security modules the
> >>> ability to check the validity of a signature against supplied data, by
> >>> using system-provided keys as trust anchor.
> >>>
> >>> The new helper 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 specify the identifier of the keyring containing the keys
> >>> for signature verification: 0 for the primary keyring (immutable keyring of
> >>> system keys); 1 for both the primary and secondary keyring (where keys can
> >>> be added only if they are vouched for by existing keys in those keyrings);
> >>> 2 for the platform keyring (primarily used by the integrity subsystem to
> >>> verify a kexec'ed kerned image and, possibly, the initramfs signature);
> >>> 0xffff for the session keyring (for testing purposes).
> >>>
> >>> The caller should also specify the type of signature. Currently only PKCS#7
> >>> is supported.
> >>>
> >>> Since the maximum number of parameters of an eBPF helper is 5, the keyring
> >>> and signature types share one (keyring ID: low 16 bits, signature type:
> >>> high 16 bits).
> >>>
> >>> Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> >>> Reported-by: kernel test robot <lkp@intel.com> (cast warning)
> >>> ---
> >>> include/uapi/linux/bpf.h | 17 +++++++++++++
> >>> kernel/bpf/bpf_lsm.c | 46 ++++++++++++++++++++++++++++++++++
> >>> tools/include/uapi/linux/bpf.h | 17 +++++++++++++
> >>> 3 files changed, 80 insertions(+)
> >>>
> >>> diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> >>> index f4009dbdf62d..97521857e44a 100644
> >>> --- a/include/uapi/linux/bpf.h
> >>> +++ b/include/uapi/linux/bpf.h
> >>> @@ -5249,6 +5249,22 @@ union bpf_attr {
> >>> * Pointer to the underlying dynptr data, NULL if the dynptr is
> >>> * read-only, if the dynptr is invalid, or if the offset and length
> >>> * is out of bounds.
> >>> + *
> >>> + * long bpf_verify_signature(u8 *data, u32 datalen, u8 *sig, u32 siglen, u32
> >> info)
> >>> + * Description
> >>> + * Verify a signature of length *siglen* against the supplied data
> >>> + * with length *datalen*. *info* contains the keyring identifier
> >>> + * (low 16 bits) and the signature type (high 16 bits). The keyring
> >>> + * identifier can have the following values (some defined in
> >>> + * verification.h): 0 for the primary keyring (immutable keyring of
> >>> + * system keys); 1 for both the primary and secondary keyring
> >>> + * (where keys can be added only if they are vouched for by
> >>> + * existing keys in those keyrings); 2 for the platform keyring
> >>> + * (primarily used by the integrity subsystem to verify a kexec'ed
> >>> + * kerned image and, possibly, the initramfs signature); 0xffff for
> >>> + * the session keyring (for testing purposes).
> >>> + * Return
> >>> + * 0 on success, a negative value on error.
> >>> */
> >>> #define __BPF_FUNC_MAPPER(FN) \
> >>> FN(unspec), \
> >>> @@ -5455,6 +5471,7 @@ union bpf_attr {
> >>> FN(dynptr_read), \
> >>> FN(dynptr_write), \
> >>> FN(dynptr_data), \
> >>> + FN(verify_signature), \
> >>> /* */
> >>>
> >>> /* integer value in 'imm' field of BPF_CALL instruction selects which helper
> >>> diff --git a/kernel/bpf/bpf_lsm.c b/kernel/bpf/bpf_lsm.c
> >>> index c1351df9f7ee..20bd850ea3ee 100644
> >>> --- a/kernel/bpf/bpf_lsm.c
> >>> +++ b/kernel/bpf/bpf_lsm.c
> >>> @@ -16,6 +16,8 @@
> >>> #include <linux/bpf_local_storage.h>
> >>> #include <linux/btf_ids.h>
> >>> #include <linux/ima.h>
> >>> +#include <linux/verification.h>
> >>> +#include <linux/module_signature.h>
> >>>
> >>> /* For every LSM hook that allows attachment of BPF programs, declare a
> >> nop
> >>> * function where a BPF program can be attached.
> >>> @@ -132,6 +134,46 @@ static const struct bpf_func_proto
> >> bpf_get_attach_cookie_proto = {
> >>> .arg1_type = ARG_PTR_TO_CTX,
> >>> };
> >>>
> >>> +#ifdef CONFIG_SYSTEM_DATA_VERIFICATION
> >>> +BPF_CALL_5(bpf_verify_signature, u8 *, data, u32, datalen, u8 *, sig,
> >>> + u32, siglen, u32, info)
> >>> +{
> >>> + unsigned long keyring_id = info & U16_MAX;
> >>> + enum pkey_id_type id_type = info >> 16;
> >>> + const struct cred *cred = current_cred();
> >>> + struct key *keyring;
> >>> +
> >>> + if (keyring_id > (unsigned long)VERIFY_USE_PLATFORM_KEYRING &&
> >>> + keyring_id != U16_MAX)
> >>> + return -EINVAL;
> >>> +
> >>> + keyring = (keyring_id == U16_MAX) ?
> >>> + cred->session_keyring : (struct key *)keyring_id;
> >>> +
> >>> + switch (id_type) {
> >>> + case PKEY_ID_PKCS7:
> >>> + return verify_pkcs7_signature(data, datalen, sig, siglen,
> >>> + keyring,
> >>> +
> >> VERIFYING_UNSPECIFIED_SIGNATURE,
> >>> + NULL, NULL);
> >>> + default:
> >>> + return -EOPNOTSUPP;
> >>
> >> Question to you & KP:
> >>
> >> > Can we keep the helper generic so that it can be extended to more types of
> >> > signatures and pass the signature type as an enum?
> >>
> >> How many different signature types do we expect, say, in the next 6mo, to land
> >> here? Just thinking out loud whether it is better to keep it simple as with the
> >> last iteration where we have a helper specific to pkcs7, and if needed in future
> >> we add others. We only have the last reg as auxillary arg where we need to
> >> squeeze
> >> all info into it now. What if for other, future signature types this won't suffice?
> >
> > I would add at least another for PGP, assuming that the code will be
> > upstreamed. But I agree, the number should not be that high.
>
> If realistically expected is really just two helpers, what speaks against a
> bpf_verify_signature_pkcs7() and bpf_verify_signature_pgp() in that case, for
> sake of better user experience?
>
> Maybe one other angle.. if CONFIG_SYSTEM_DATA_VERIFICATION is enabled, it may
> not be clear whether verify_pkcs7_signature() or a verify_pgp_signature() are
> both always builtin. And then, we run into the issue again of more complex probing
> for availability of the algs compared to simple ...
>
> #if defined(CONFIG_SYSTEM_DATA_VERIFICATION) && defined(CONFIG_XYZ)
> case BPF_FUNC_verify_signature_xyz:
> return ..._proto;
> #endif
>
> ... which bpftool and others easily understand.
>
> >>> + }
> >>> +}
> >>> +
> >>> +static const struct bpf_func_proto bpf_verify_signature_proto = {
> >>> + .func = bpf_verify_signature,
> >>> + .gpl_only = false,
> >>> + .ret_type = RET_INTEGER,
> >>> + .arg1_type = ARG_PTR_TO_MEM,
> >>> + .arg2_type = ARG_CONST_SIZE_OR_ZERO,
> >>
> >> Can verify_pkcs7_signature() handle null/0 len for data* args?
> >
> > Shouldn't ARG_PTR_TO_MEM require valid memory? 0 len should
> > not be a problem.
>
> check_helper_mem_access() has:
>
> /* Allow zero-byte read from NULL, regardless of pointer type */
> if (zero_size_allowed && access_size == 0 &&
> register_is_null(reg))
> return 0;

Daniel, makes a fair point here. Alexei, what do you think?

I wonder if some "future" signature verification would need even more
/ different arguments so a unified bpf_verify_signature might get more
complex / not easy to extend.


>
> So NULL/0 pair can be passed. Maybe good to add these corner cases to the test_progs
> selftest additions then if it's needed.
>
> >>> + .arg3_type = ARG_PTR_TO_MEM,
> >>> + .arg4_type = ARG_CONST_SIZE_OR_ZERO,
> >>
> >> Ditto for sig* args?
> >>
> >>> + .arg5_type = ARG_ANYTHING,
> >>> + .allowed = bpf_ima_inode_hash_allowed,
> >>> +};
> >>> +#endif
> >>> +
> >>> static const struct bpf_func_proto *
> >>> bpf_lsm_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
> >>> {
> >>> @@ -158,6 +200,10 @@ bpf_lsm_func_proto(enum bpf_func_id func_id,
> >> const struct bpf_prog *prog)
> >>> return prog->aux->sleepable ? &bpf_ima_file_hash_proto :
> >> NULL;
> >>> case BPF_FUNC_get_attach_cookie:
> >>> return bpf_prog_has_trampoline(prog) ?
> >> &bpf_get_attach_cookie_proto : NULL;
> >>> +#ifdef CONFIG_SYSTEM_DATA_VERIFICATION
> >>> + case BPF_FUNC_verify_signature:
> >>> + return prog->aux->sleepable ? &bpf_verify_signature_proto :
> >> NULL;
> >>> +#endif
> >>> default:
> >>> return tracing_prog_func_proto(func_id, prog);
> >>> }
>

\
 
 \ /
  Last update: 2022-06-11 01:54    [W:0.880 / U:0.296 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site