Re: [PATCH v2] bpf: crypto: snapshot params before string validation

[sashiko-bot@kernel.org]

Sashiko AI review found 2 potential issue(s):
- [Medium] Cryptographic key material is left on the kernel stack without being zeroized.
- [Low] Unnecessary and technically incorrect file descriptor validation for a skeleton-generated program.
--

commit 4025cf60c980030df8d7712c14d6f5b70e1660b8
Author: Pengpeng Hou <pengpeng@iscas.ac.cn>

bpf: crypto: snapshot params before string validation

This commit updates bpf_crypto_ctx_create() to snapshot the BPF-provided
parameters into a local stack variable before validating them. This prevents
issues where BPF programs might modify the memory during validation. It also
adds a selftest for testing invalid crypto algorithms.

> diff --git a/kernel/bpf/crypto.c b/kernel/bpf/crypto.c
> index 51f89cecefb4dc..e2dc3dfb4d8540 100644
> --- a/kernel/bpf/crypto.c
> +++ b/kernel/bpf/crypto.c
[ ... ]
> @@ -147,31 +147,46 @@ bpf_crypto_ctx_create(const struct bpf_crypto_params *params, u32 params__sz,
>  		      int *err)
>  {
>  	const struct bpf_crypto_type *type;
> +	struct bpf_crypto_params params_copy;
>  	struct bpf_crypto_ctx *ctx;
[ ... ]
> +	params_copy = *params;

Does this leave plain-text cryptographic keys on the kernel stack?

The struct bpf_crypto_params contains key material. By copying the caller's
structure into the local params_copy variable, the key is placed on the
stack. Should params_copy be cleared using memzero_explicit() before the
function returns on both success and error paths to prevent potential
information leaks?

[ ... ]
> diff --git a/tools/testing/selftests/bpf/prog_tests/crypto_sanity.c b/tools/testing/selftests/bpf/prog_tests/crypto_sanity.c
> index 42bd07f7218dc3..044711e7263a8f 100644
> --- a/tools/testing/selftests/bpf/prog_tests/crypto_sanity.c
> +++ b/tools/testing/selftests/bpf/prog_tests/crypto_sanity.c
[ ... ]
> @@ -117,6 +117,19 @@ void test_crypto_sanity(void)
>  	udp_test_port = skel->data->udp_test_port;
>  	memcpy(skel->bss->key, crypto_key, sizeof(crypto_key));
>  	snprintf(skel->bss->algo, 128, "%s", algo);
> +
> +	pfd = bpf_program__fd(skel->progs.skb_crypto_setup_bad_algo);
> +	if (!ASSERT_GT(pfd, 0, "skb_crypto_setup_bad_algo fd"))

Is this file descriptor check necessary?

After a successful skeleton open_and_load(), the skeleton-generated program
fields are guaranteed to contain valid file descriptors, making manual
validation redundant.

Additionally, 0 is a valid file descriptor in Linux. If the test program
received FD 0, using ASSERT_GT(pfd, 0) would incorrectly cause the test to
fail. Could this be removed?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260424070105.1-bpf-crypto-v2-pengpeng@iscas.ac.cn?part=1