Re: [PATCH v12 04/15] crypto: ti - Fix use-after-free of dev_data on DTHEv2 driver removal

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Medium] crypto: ti - missing dev_data refcnt drain on dthe_probe()
  error path
--

commit 671be96b9ffa7abb434446440cf647e7e1f76348
Author: T Pratham <t-pratham@ti.com>

crypto: ti - Fix use-after-free of dev_data on DTHEv2 driver removal

This commit moves the dthe_data pointer from the transform context to the
request context, binds transforms to a reference-counted struct device,
and adds a kref and completion to struct dthe_data so dthe_remove() waits
for in-flight requests to drain before tearing down the device.

> diff --git a/drivers/crypto/ti/dthev2-common.c b/drivers/crypto/ti/dthev2-common.c
> index 911e6bbe4d241..cd4626ed4c39a 100644
> --- a/drivers/crypto/ti/dthev2-common.c
> +++ b/drivers/crypto/ti/dthev2-common.c

[ ... ]

> @@ -184,6 +219,7 @@ static int dthe_probe(struct platform_device *pdev)
>  probe_register_algs_err:
>  	spin_lock_bh(&dthe_dev_list.lock);
>  	list_del(&dev_data->list);
> +	platform_set_drvdata(pdev, NULL);
>  	spin_unlock_bh(&dthe_dev_list.lock);
>  probe_engine_start_err:
>  	crypto_engine_exit(dev_data->engine);

[Severity: Medium]
Does the probe_register_algs_err rollback path also need to drop the
initial reference and wait for drain_compl like dthe_remove() does?

dthe_probe() publishes dev_data in drvdata and dthe_dev_list before calling
dthe_register_algs(), which registers skcipher algorithms first and AEAD
algorithms second:

drivers/crypto/ti/dthev2-aes.c:dthe_register_aes_algs() {
	ret = crypto_engine_register_skciphers(cipher_algs, ARRAY_SIZE(cipher_algs));
	if (ret)
		return ret;
	ret = crypto_engine_register_aeads(aead_algs, ARRAY_SIZE(aead_algs));
	if (ret)
		crypto_engine_unregister_skciphers(cipher_algs, ARRAY_SIZE(cipher_algs));

	return ret;
}

If registering one of the later algorithms fails after an earlier algorithm
has already been picked up by cryptomgr_test or another user, a request can
remain in flight after crypto_engine_unregister_skciphers() returns:

CPU0                              CPU1
dthe_probe()
  platform_set_drvdata(pdev, dev_data)
  list_add(&dev_data->list, ...)
  dthe_register_algs()
    crypto_engine_register_skciphers()
                                  dthe_cipher_init_tfm()
                                    ctx->dev = dthe_get_dev()
                                  dthe_aes_crypt()
                                    dev_data = dthe_get_drvdata(ctx->dev)
    crypto_engine_register_aeads() -> fails
    crypto_engine_unregister_skciphers()
  list_del(&dev_data->list)
  platform_set_drvdata(pdev, NULL)
  crypto_engine_exit(dev_data->engine)
  return ret; (frees devm dev_data)
                                    crypto_transfer_skcipher_request_to_engine()
                                    dthe_put_drvdata(dev_data)
                                      <-- use-after-free

Calling dthe_put_drvdata(dev_data) and
wait_for_completion(&dev_data->drain_compl) before probe_engine_start_err
would wait for any in-flight requests to finish before tearing down the
engine and freeing dev_data.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261107160631.2592763-1-t-pratham@ti.com?part=4