Re: [PATCH v3 1/2] bpf: Add bpf_verify_signature() helper

[Daniel Borkmann <daniel@iogearbox.net>]

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);
>>>    	}