lkml.org 
[lkml]   [2022]   [Jun]   [10]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
SubjectRe: [PATCH v3 1/2] bpf: Add bpf_verify_signature() helper
From
Date
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;

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-10 17:16    [W:0.049 / U:1.288 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site