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

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

[Li Ximo]

From: Wang Haoran <haoranwangsec@gmail.com>

While auditing list_first_entry() users we found that dthe_get_dev()
calls list_first_entry() on the global dthe_dev_list.dev_list without
checking whether the list is empty. list_first_entry() never returns
NULL: on an empty list head->next == head, so container_of() produces
a bogus non-NULL pointer into the static dthe_dev_list structure. The
existing "if (dev_data)" check is therefore dead code - it can never
be false - and list_move_tail(&dev_data->list, ...) then reads and
writes through that wild pointer, corrupting the list head, after
which callers use the bogus pointer for engine/MMIO accesses.

The empty list is reachable because dthe_remove() removes the device
from the list (list_del) before unregistering the crypto algorithms
(dthe_unregister_algs). On single-device SoCs the list is empty from
that point on, while the algorithms remain registered, so a new
tfm (ctx->dev_data not yet cached) can still reach dthe_get_dev() and
perform the lookup on the empty list. K3 DTHE is a platform device, so
unbind/remove is enough to open this window.

Fix this with two changes: switch dthe_get_dev() to
list_first_entry_or_null(), which makes the existing NULL check
effective and returns NULL when no device is registered; and reorder
dthe_remove() to unregister the algorithms before removing the device
from the list, mirroring the probe order (list_add before
dthe_register_algs) and preventing new requests from being dispatched
once the device is gone. This was found by code audit; we do not have
K3 hardware to reproduce it.

Note that the nine dthe_get_dev() call sites in dthev2-aes.c do not
check for a NULL return. With the remove() reordering the empty-list
window is closed, so the NULL return is purely defensive; adding NULL
checks to the callers may nevertheless be a worthwhile follow-up.

Reported-by: Wang Haoran <haoranwangsec@gmail.com>
Reported-by: Li Ximo <lixicaicai@gmail.com>
Signed-off-by: Wang Haoran <haoranwangsec@gmail.com>
---
 drivers/crypto/ti/dthev2-common.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/crypto/ti/dthev2-common.c b/drivers/crypto/ti/dthev2-common.c
index ba6cde75d..099c0b923 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();
-
 	crypto_engine_exit(dev_data->engine);
 
 	dma_release_channel(dev_data->dma_aes_rx);
-- 
2.30.2

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

[sashiko-bot]

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

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

[T Pratham]

On 10/28/26 09:33, Li Ximo wrote:
> From: Wang Haoran <haoranwangsec@gmail.com>
> 
> While auditing list_first_entry() users we found that dthe_get_dev()
> calls list_first_entry() on the global dthe_dev_list.dev_list without
> checking whether the list is empty. list_first_entry() never returns
> NULL: on an empty list head->next == head, so container_of() produces
> a bogus non-NULL pointer into the static dthe_dev_list structure. The
> existing "if (dev_data)" check is therefore dead code - it can never
> be false - and list_move_tail(&dev_data->list, ...) then reads and
> writes through that wild pointer, corrupting the list head, after
> which callers use the bogus pointer for engine/MMIO accesses.
> 
> The empty list is reachable because dthe_remove() removes the device
> from the list (list_del) before unregistering the crypto algorithms
> (dthe_unregister_algs). On single-device SoCs the list is empty from
> that point on, while the algorithms remain registered, so a new
> tfm (ctx->dev_data not yet cached) can still reach dthe_get_dev() and
> perform the lookup on the empty list. K3 DTHE is a platform device, so
> unbind/remove is enough to open this window.
> 
> Fix this with two changes: switch dthe_get_dev() to
> list_first_entry_or_null(), which makes the existing NULL check
> effective and returns NULL when no device is registered; and reorder
> dthe_remove() to unregister the algorithms before removing the device
> from the list, mirroring the probe order (list_add before
> dthe_register_algs) and preventing new requests from being dispatched
> once the device is gone. This was found by code audit; we do not have
> K3 hardware to reproduce it.
> 
> Note that the nine dthe_get_dev() call sites in dthev2-aes.c do not
> check for a NULL return. With the remove() reordering the empty-list
> window is closed, so the NULL return is purely defensive; adding NULL
> checks to the callers may nevertheless be a worthwhile follow-up.
> 
> Reported-by: Wang Haoran <haoranwangsec@gmail.com>
> Reported-by: Li Ximo <lixicaicai@gmail.com>
> Signed-off-by: Wang Haoran <haoranwangsec@gmail.com>
> ---

Hi Wang

Thanks for the patch. However, this fix is already in works in this series:

[PATCH v10 00/15] Fix several issues in DTHEv2 driver
https://lore.kernel.org/all/20261016124456.1802080-1-t-pratham@ti.com/

Specifically this is the patch:
https://lore.kernel.org/all/20261016124456.1802080-2-t-pratham@ti.com/

-- 
Regards
T Pratham <t-pratham@ti.com>