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

[sashiko-bot@kernel.org]

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