[PATCH] bpf: crypto: reject unterminated type and algorithm names

[PATCH] bpf: crypto: reject unterminated type and algorithm names

[Pengpeng Hou]

bpf_crypto_ctx_create() validates the overall size of
struct bpf_crypto_params, but it does not verify that the fixed-width
type[14] and algo[128] fields are NUL-terminated before passing them to
string consumers.

A caller can therefore fill either field without a terminator and cause
bpf_crypto_get_type(), has_algo(), or alloc_tfm() to read past the end
of the fixed buffer.

Reject parameter blocks whose type or algorithm name does not contain a
terminating NUL within the advertised field width.

Fixes: 3e1c6f35409f ("bpf: make common crypto API for TC/XDP programs")
Cc: stable@vger.kernel.org

Signed-off-by: Pengpeng Hou <pengpeng@iscas.ac.cn>
---
 kernel/bpf/crypto.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/kernel/bpf/crypto.c b/kernel/bpf/crypto.c
index 51f89cecefb4..8732689803a6 100644
--- a/kernel/bpf/crypto.c
+++ b/kernel/bpf/crypto.c
@@ -155,6 +155,12 @@ bpf_crypto_ctx_create(const struct bpf_crypto_params *params, u32 params__sz,
 		return NULL;
 	}
 
+	if (strnlen(params->type, sizeof(params->type)) == sizeof(params->type) ||
+	    strnlen(params->algo, sizeof(params->algo)) == sizeof(params->algo)) {
+		*err = -EINVAL;
+		return NULL;
+	}
+
 	type = bpf_crypto_get_type(params->type);
 	if (IS_ERR(type)) {
 		*err = PTR_ERR(type);
-- 
2.50.1 (Apple Git-155)

Re: [PATCH] bpf: crypto: reject unterminated type and algorithm names

[sashiko-bot]

Sashiko AI review found 1 potential issue(s):
- [Critical] In-place validation of `struct bpf_crypto_params` is vulnerable to Time-of-Check to Time-of-Use (TOCTOU) race conditions, allowing bypassing of bounds checks and leading to NULL pointer dereferences and out-of-bounds reads.
--

commit 3ccfc91c2a01e60f8c9cc4188d35f3073228f1db
Author: Pengpeng Hou <pengpeng@iscas.ac.cn>

bpf: crypto: reject unterminated type and algorithm names

This commit attempts to fix out-of-bounds reads by adding string length checks
for the type and algo fields in bpf_crypto_ctx_create(). It returns -EINVAL if
the strings are not NUL-terminated.

> diff --git a/kernel/bpf/crypto.c b/kernel/bpf/crypto.c
> --- a/kernel/bpf/crypto.c
> +++ b/kernel/bpf/crypto.c
[ ... ]
> @@ -155,6 +155,12 @@ bpf_crypto_ctx_create(const struct bpf_crypto_params *params, u32 params__sz,
>  		return NULL;
>  	}
>  
> +	if (strnlen(params->type, sizeof(params->type)) == sizeof(params->type) ||
> +	    strnlen(params->algo, sizeof(params->algo)) == sizeof(params->algo)) {

Is there a potential time-of-check to time-of-use race condition here?
If the params pointer refers to a BPF map value, user space or other CPUs
could concurrently modify the map data.

Could the NUL terminator be overwritten immediately after these strnlen()
checks pass, causing downstream consumers like bpf_crypto_get_type() to still
read out-of-bounds?

> +		*err = -EINVAL;
> +		return NULL;
> +	}
> +
>  	type = bpf_crypto_get_type(params->type);

Looking at the rest of bpf_crypto_ctx_create() in the original code, the same
in-place validation pattern appears to affect other fields:

kernel/bpf/crypto.c:bpf_crypto_ctx_create() {
    ...
    if (!!params->authsize ^ !!type->setauthsize) {
        ...
    }
    ...
    if (params->authsize) {
        *err = type->setauthsize(ctx->tfm, params->authsize);
    ...
}

If type->setauthsize is NULL, and params->authsize is initially 0, the XOR
check passes. If params->authsize is concurrently modified to a non-zero
value right after, does the later if (params->authsize) condition evaluate
to true and dereference a NULL pointer?

Similarly, params->key_len is validated against sizeof(params->key):

kernel/bpf/crypto.c:bpf_crypto_ctx_create() {
    ...
    if (!params->key_len || params->key_len > sizeof(params->key)) {
        ...
    }
    ...
    *err = type->setkey(ctx->tfm, params->key, params->key_len);
    ...
}

Could a concurrent modification to params->key_len after the check lead to an
out-of-bounds read in type->setkey()?

Should the entire struct bpf_crypto_params be copied to a local stack buffer
before any validation is performed to prevent these races?

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

Re: [PATCH] bpf: crypto: reject unterminated type and algorithm names

[Vadim Fedorenko]

On 17.04.2026 08:31, Pengpeng Hou wrote:
> bpf_crypto_ctx_create() validates the overall size of
> struct bpf_crypto_params, but it does not verify that the fixed-width
> type[14] and algo[128] fields are NUL-terminated before passing them to
> string consumers.
> 
> A caller can therefore fill either field without a terminator and cause
> bpf_crypto_get_type(), has_algo(), or alloc_tfm() to read past the end
> of the fixed buffer.
How can this happen for static defined type/algo structures?

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

[Pengpeng Hou]

bpf_crypto_ctx_create() receives a BPF-supplied params pointer. The
current selftests use static initializers, but BPF programs can also
build the struct in writable BPF memory before calling the kfunc. The
verifier checks that the memory is accessible; it does not prove that
the fixed type[] and algo[] fields are NUL-terminated strings.

Copy the params once into a local snapshot, validate the reserved fields
and fixed-width strings there, and then use the same snapshot for all
later checks and crypto API calls. This also keeps key_len and authsize
stable across validation and use if params points at mutable BPF memory.

Add a selftest that fills algo[] completely and expects -EINVAL.

Fixes: 3e1c6f35409f ("bpf: make common crypto API for TC/XDP programs")
Signed-off-by: Pengpeng Hou <pengpeng@iscas.ac.cn>
---
Changes since v1:
- copy struct bpf_crypto_params into a local snapshot before validation
- use the snapshot for all string, key_len and authsize checks and uses
- add a BPF selftest for an unterminated algorithm name

diff --git a/kernel/bpf/crypto.c b/kernel/bpf/crypto.c
index 51f89cecefb4..e2dc3dfb4d85 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;
 
-	if (!params || params->reserved[0] || params->reserved[1] ||
-	    params__sz != sizeof(struct bpf_crypto_params)) {
+	if (!params || params__sz != sizeof(params_copy)) {
 		*err = -EINVAL;
 		return NULL;
 	}
 
-	type = bpf_crypto_get_type(params->type);
+	params_copy = *params;
+
+	if (params_copy.reserved[0] || params_copy.reserved[1]) {
+		*err = -EINVAL;
+		return NULL;
+	}
+
+	if (strnlen(params_copy.type, sizeof(params_copy.type)) ==
+	    sizeof(params_copy.type) ||
+	    strnlen(params_copy.algo, sizeof(params_copy.algo)) ==
+	    sizeof(params_copy.algo)) {
+		*err = -EINVAL;
+		return NULL;
+	}
+
+	type = bpf_crypto_get_type(params_copy.type);
 	if (IS_ERR(type)) {
 		*err = PTR_ERR(type);
 		return NULL;
 	}
 
-	if (!type->has_algo(params->algo)) {
+	if (!type->has_algo(params_copy.algo)) {
 		*err = -EOPNOTSUPP;
 		goto err_module_put;
 	}
 
-	if (!!params->authsize ^ !!type->setauthsize) {
+	if (!!params_copy.authsize ^ !!type->setauthsize) {
 		*err = -EOPNOTSUPP;
 		goto err_module_put;
 	}
 
-	if (!params->key_len || params->key_len > sizeof(params->key)) {
+	if (!params_copy.key_len || params_copy.key_len > sizeof(params_copy.key)) {
 		*err = -EINVAL;
 		goto err_module_put;
 	}
@@ -183,19 +198,19 @@ bpf_crypto_ctx_create(const struct bpf_crypto_params *params, u32 params__sz,
 	}
 
 	ctx->type = type;
-	ctx->tfm = type->alloc_tfm(params->algo);
+	ctx->tfm = type->alloc_tfm(params_copy.algo);
 	if (IS_ERR(ctx->tfm)) {
 		*err = PTR_ERR(ctx->tfm);
 		goto err_free_ctx;
 	}
 
-	if (params->authsize) {
-		*err = type->setauthsize(ctx->tfm, params->authsize);
+	if (params_copy.authsize) {
+		*err = type->setauthsize(ctx->tfm, params_copy.authsize);
 		if (*err)
 			goto err_free_tfm;
 	}
 
-	*err = type->setkey(ctx->tfm, params->key, params->key_len);
+	*err = type->setkey(ctx->tfm, params_copy.key, params_copy.key_len);
 	if (*err)
 		goto err_free_tfm;
 
diff --git a/tools/testing/selftests/bpf/prog_tests/crypto_sanity.c b/tools/testing/selftests/bpf/prog_tests/crypto_sanity.c
index 42bd07f7218d..044711e7263a 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"))
+		goto fail;
+
+	err = bpf_prog_test_run_opts(pfd, &opts);
+	if (!ASSERT_OK(err, "skb_crypto_setup_bad_algo") ||
+	    !ASSERT_OK(opts.retval, "skb_crypto_setup_bad_algo retval"))
+		goto fail;
+
+	if (!ASSERT_OK(skel->bss->status, "skb_crypto_setup_bad_algo status"))
+		goto fail;
+
 	pfd = bpf_program__fd(skel->progs.skb_crypto_setup);
 	if (!ASSERT_GT(pfd, 0, "skb_crypto_setup fd"))
 		goto fail;
diff --git a/tools/testing/selftests/bpf/progs/crypto_sanity.c b/tools/testing/selftests/bpf/progs/crypto_sanity.c
index dfd8a258f14a..32977b797042 100644
--- a/tools/testing/selftests/bpf/progs/crypto_sanity.c
+++ b/tools/testing/selftests/bpf/progs/crypto_sanity.c
@@ -82,6 +82,34 @@ int skb_crypto_setup(void *ctx)
 	return 0;
 }
 
+SEC("syscall")
+int skb_crypto_setup_bad_algo(void *ctx)
+{
+	struct bpf_crypto_params params = {
+		.type = "skcipher",
+		.key_len = 16,
+	};
+	struct bpf_crypto_ctx *cctx;
+	int err = 0;
+
+	status = 0;
+
+	__builtin_memset(params.algo, 'a', sizeof(params.algo));
+	__builtin_memcpy(&params.key, key, sizeof(key));
+
+	cctx = bpf_crypto_ctx_create(&params, sizeof(params), &err);
+	if (cctx) {
+		bpf_crypto_ctx_release(cctx);
+		status = -EIO;
+		return 0;
+	}
+
+	if (err != -EINVAL)
+		status = err;
+
+	return 0;
+}
+
 SEC("tc")
 int decrypt_sanity(struct __sk_buff *skb)
 {
-- 
2.50.1 (Apple Git-155)

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

[sashiko-bot]

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