Re: [PATCH] crypto: ti - dthev2: fix empty-list wild pointer in dthe_get_dev()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] crypto: ti - dthev2: missing NULL check on dthe_get_dev() return value
--

commit 7858ab938a94a13ed7e28adf576e74952aceb946
Author: Wang Haoran <haoranwangsec@gmail.com>

crypto: ti - dthev2: fix empty-list wild pointer in dthe_get_dev()

This commit changes dthe_get_dev() to use list_first_entry_or_null() when
looking up a device on dthe_dev_list.dev_list and reorders dthe_remove()
to call dthe_unregister_algs() before removing the device from the list.

> diff --git a/drivers/crypto/ti/dthev2-common.c b/drivers/crypto/ti/dthev2-common.c
> index ba6cde75d3611..099c0b92368d6 100644
> --- a/drivers/crypto/ti/dthev2-common.c
> +++ b/drivers/crypto/ti/dthev2-common.c
> @@ -39,7 +39,8 @@ struct dthe_data *dthe_get_dev(struct dthe_tfm_ctx *ctx)
>  		return ctx->dev_data;
>  
>  	spin_lock_bh(&dthe_dev_list.lock);
> -	dev_data = list_first_entry(&dthe_dev_list.dev_list, struct dthe_data, list);
> +	dev_data = list_first_entry_or_null(&dthe_dev_list.dev_list,
> +					    struct dthe_data, list);
>  	if (dev_data)
>  		list_move_tail(&dev_data->list, &dthe_dev_list.dev_list);
>  	spin_unlock_bh(&dthe_dev_list.lock);
> @@ -200,12 +201,12 @@ static void dthe_remove(struct platform_device *pdev)
>  {
>  	struct dthe_data *dev_data = platform_get_drvdata(pdev);
>  
> +	dthe_unregister_algs();
> +
>  	spin_lock(&dthe_dev_list.lock);
>  	list_del(&dev_data->list);
>  	spin_unlock(&dthe_dev_list.lock);
>  
> -	dthe_unregister_algs();

[Severity: High]
Does calling dthe_unregister_algs() before list_del(&dev_data->list) in
dthe_remove() completely close the empty-list window, or can callers in
drivers/crypto/ti/dthev2-aes.c still receive NULL from dthe_get_dev() and
dereference a NULL dev_data pointer?

In the crypto API, crypto_alloc_tfm_node() holds crypto_alg_sem only inside
crypto_find_alg() while looking up the algorithm and taking a module
reference, then releases crypto_alg_sem before calling
crypto_create_tfm_node() -> frontend->init_tfm(tfm).

If dthe_remove() runs concurrently (for example via platform device unbind),
dthe_unregister_algs() -> crypto_unregister_alg() unlinks the algorithms
under crypto_alg_sem without waiting for in-flight crypto_alloc_tfm_node()
callers that already finished crypto_find_alg(), and then executes
list_del(&dev_data->list) to empty dthe_dev_list.dev_list. A similar window
also exists in dthe_probe() if dthe_register_aes_algs() fails partway after
crypto_register_alg() has spawned asynchronous cryptomgr_test kthreads and
the error path removes dev_data from dthe_dev_list.dev_list.

When an in-flight init_tfm callback in drivers/crypto/ti/dthev2-aes.c runs
after the list becomes empty, dthe_get_dev(ctx) now returns NULL to callers
that do not check for NULL:

drivers/crypto/ti/dthev2-aes.c:dthe_cipher_init_tfm() {
	struct dthe_data *dev_data = dthe_get_dev(ctx);

	ctx->dev_data = dev_data;
	ctx->keylen = 0;

	return 0;
}

drivers/crypto/ti/dthev2-aes.c:dthe_cipher_init_tfm_fallback() {
	struct dthe_data *dev_data = dthe_get_dev(ctx);
	const char *alg_name = crypto_tfm_alg_name(crypto_skcipher_tfm(tfm));

	ctx->dev_data = dev_data;
	ctx->keylen = 0;

	ctx->skcipher_fb = crypto_alloc_sync_skcipher(alg_name, 0,
						      CRYPTO_ALG_NEED_FALLBACK);
	if (IS_ERR(ctx->skcipher_fb)) {
		dev_err(dev_data->dev, "fallback driver %s couldn't be loaded\n",
			alg_name);
		...
}

drivers/crypto/ti/dthev2-aes.c:dthe_aead_init_tfm() {
	struct dthe_data *dev_data = dthe_get_dev(ctx);

	ctx->dev_data = dev_data;

	const char *alg_name = crypto_tfm_alg_name(crypto_aead_tfm(tfm));

	ctx->aead_fb = crypto_alloc_sync_aead(alg_name, 0,
					      CRYPTO_ALG_NEED_FALLBACK);
	if (IS_ERR(ctx->aead_fb)) {
		dev_err(dev_data->dev, "fallback driver %s couldn't be loaded\n",
			alg_name);
		...
}

If fallback allocation fails in dthe_cipher_init_tfm_fallback() or
dthe_aead_init_tfm(), dev_err(dev_data->dev, ...) immediately dereferences
a NULL dev_data pointer. Otherwise, init_tfm succeeds with
ctx->dev_data == NULL, and subsequent requests in dthe_aes_crypt() or
dthe_aead_crypt() get NULL from dthe_get_dev(ctx) and dereference
dev_data->engine:

drivers/crypto/ti/dthev2-aes.c:dthe_aes_crypt() {
	struct dthe_data *dev_data = dthe_get_dev(ctx);
	...
	engine = dev_data->engine;
	return crypto_transfer_skcipher_request_to_engine(engine, req);
}

drivers/crypto/ti/dthev2-aes.c:dthe_aead_crypt() {
	struct dthe_data *dev_data = dthe_get_dev(ctx);
	...
	engine = dev_data->engine;
	return crypto_transfer_aead_request_to_engine(engine, req);
}

Should the init_tfm callbacks in drivers/crypto/ti/dthev2-aes.c check for
!dev_data and return -ENODEV?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261028040319.171479-1-lixicaicai@gmail.com?part=1