Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Thu, 16 Jun 2022 20:46:17 -0700
From:   Alexei Starovoitov <alexei.starovoitov@gmail.com>
To:     Roberto Sassu <roberto.sassu@huawei.com>
Cc:     ast@kernel.org, daniel@iogearbox.net, andrii@kernel.org,
        kpsingh@kernel.org, john.fastabend@gmail.com,
        songliubraving@fb.com, kafai@fb.com, yhs@fb.com,
        bpf@vger.kernel.org, netdev@vger.kernel.org,
        linux-kselftest@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [RESEND][PATCH v4 2/4] bpf: Add bpf_request_key_by_id() helper
Message-ID: <20220617034617.db23phfavuhqx4vi@MacBook-Pro-3.local>
References: <20220614130621.1976089-1-roberto.sassu@huawei.com>
 <20220614130621.1976089-3-roberto.sassu@huawei.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20220614130621.1976089-3-roberto.sassu@huawei.com>
Precedence: bulk

On Tue, Jun 14, 2022 at 03:06:19PM +0200, Roberto Sassu wrote:
> Add the bpf_request_key_by_id() helper, so that an eBPF program can obtain
> a suitable key pointer to pass to the bpf_verify_pkcs7_signature() helper,
> to be introduced in a later patch.
> 
> The passed identifier can have the following values: 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); ULONG_MAX for the session keyring (for
> testing purposes).
> 
> Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> ---
>  include/uapi/linux/bpf.h       | 17 +++++++++++++++++
>  kernel/bpf/bpf_lsm.c           | 30 ++++++++++++++++++++++++++++++
>  scripts/bpf_doc.py             |  2 ++
>  tools/include/uapi/linux/bpf.h | 17 +++++++++++++++++
>  4 files changed, 66 insertions(+)
> 
> diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> index f4009dbdf62d..dfd93e0e0759 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.
> + *
> + * struct key *bpf_request_key_by_id(unsigned long id)
> + *	Description
> + *		Request a keyring by *id*.
> + *
> + *		*id* 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); ULONG_MAX
> + *		for the session keyring (for testing purposes).

It's never ok to add something like this to uapi 'for testing purposes'.
If it's not useful in general it should not be a part of api.

> + *	Return
> + *		A non-NULL pointer if *id* is valid and not 0, a NULL pointer
> + *		otherwise.
>   */
>  #define __BPF_FUNC_MAPPER(FN)		\
>  	FN(unspec),			\
> @@ -5455,6 +5471,7 @@ union bpf_attr {
>  	FN(dynptr_read),		\
>  	FN(dynptr_write),		\
>  	FN(dynptr_data),		\
> +	FN(request_key_by_id),		\
>  	/* */
>  
>  /* 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..e1911812398b 100644
> --- a/kernel/bpf/bpf_lsm.c
> +++ b/kernel/bpf/bpf_lsm.c
> @@ -16,6 +16,7 @@
>  #include <linux/bpf_local_storage.h>
>  #include <linux/btf_ids.h>
>  #include <linux/ima.h>
> +#include <linux/verification.h>
>  
>  /* For every LSM hook that allows attachment of BPF programs, declare a nop
>   * function where a BPF program can be attached.
> @@ -132,6 +133,31 @@ static const struct bpf_func_proto bpf_get_attach_cookie_proto = {
>  	.arg1_type	= ARG_PTR_TO_CTX,
>  };
>  
> +#ifdef CONFIG_KEYS
> +BTF_ID_LIST_SINGLE(bpf_request_key_by_id_btf_ids, struct, key)
> +
> +BPF_CALL_1(bpf_request_key_by_id, unsigned long, id)
> +{
> +	const struct cred *cred = current_cred();
> +
> +	if (id > (unsigned long)VERIFY_USE_PLATFORM_KEYRING && id != ULONG_MAX)
> +		return (unsigned long)NULL;
> +
> +	if (id == ULONG_MAX)
> +		return (unsigned long)cred->session_keyring;
> +
> +	return id;

It needs to do a proper lookup.
Why cannot it do lookup_user_key ?
The helper needs 'flags' arg too.
Please think hard of extensibility and long term usefulness of api.
At present this api feels like it was 'let me just hack something quickly'. Not ok.