[PATCH] hwrng: cctrng - cancel deferred works on unbind to avoid use-after-free

[PATCH] hwrng: cctrng - cancel deferred works on unbind to avoid use-after-free

[hyunjungg]

From: Hyunjung Ko <hj351016@gmail.com>

cctrng schedules two deferred works that dereference the driver data via
container_of(): compwork (cc_trng_compwork_handler) is queued from the
interrupt handler cc_isr() on an RNG completion interrupt, and startwork
(cc_trng_startwork_handler) is queued from the hwrng read path.

The driver data is allocated with devm_kzalloc() and nothing ever cancels
these works. On unbind, cctrng_remove() only runs the PM teardown, and
devres then releases the hwrng registration, the IRQ and finally the
driver data. free_irq() stops further interrupts but does not cancel a
work the handler has already queued, so a completion interrupt arriving
shortly before removal can leave compwork pending and have
cc_trng_compwork_handler() run against the freed cctrng_drvdata
(use-after-free). The hwrng core does not drain this driver-private work.

Register the works with devm_work_autocancel() instead of INIT_WORK().
They are set up before devm_request_irq() and devm_hwrng_register(), so on
unwind devres unregisters the hwrng and frees the IRQ first - after which
neither work can be scheduled any more - and then cancels the works while
the driver data is still valid. The handlers do not re-queue themselves,
so a single cancel drains them.

Found by static analysis and code review; not runtime-reproduced (needs
Arm CryptoCell TRNG hardware and a completion interrupt racing unbind).

Fixes: a583ed310bb6 ("hwrng: cctrng - introduce Arm CryptoCell driver")
Cc: stable@vger.kernel.org
Signed-off-by: Hyunjung Ko <hj351016@gmail.com>
---
 drivers/char/hw_random/cctrng.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/char/hw_random/cctrng.c b/drivers/char/hw_random/cctrng.c
index 03401ee..138cc70 100644
--- a/drivers/char/hw_random/cctrng.c
+++ b/drivers/char/hw_random/cctrng.c
@@ -11,6 +11,7 @@
 #include <linux/interrupt.h>
 #include <linux/irqreturn.h>
 #include <linux/workqueue.h>
+#include <linux/devm-helpers.h>
 #include <linux/circ_buf.h>
 #include <linux/completion.h>
 #include <linux/of.h>
@@ -497,8 +498,14 @@ static int cctrng_probe(struct platform_device *pdev)
 		return dev_err_probe(dev, PTR_ERR(drvdata->clk),
 				     "Failed to get or enable the clock\n");
 
-	INIT_WORK(&drvdata->compwork, cc_trng_compwork_handler);
-	INIT_WORK(&drvdata->startwork, cc_trng_startwork_handler);
+	rc = devm_work_autocancel(dev, &drvdata->compwork,
+				  cc_trng_compwork_handler);
+	if (rc)
+		return rc;
+	rc = devm_work_autocancel(dev, &drvdata->startwork,
+				  cc_trng_startwork_handler);
+	if (rc)
+		return rc;
 	spin_lock_init(&drvdata->read_lock);
 
 	/* register the driver isr function */
-- 
2.43.0

Re: [PATCH] hwrng: cctrng - cancel deferred works on unbind to avoid use-after-free

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found no new issues
in this patch, but noticed 1 related pre-existing issue it does not
introduce:
- [High] hwrng: cctrng - runtime resume before enabling runtime PM in
  cctrng_probe() [1]
--

commit 8e1ce791cb06b2432af8b8abe4b88073b7383901
Author: Hyunjung Ko <hj351016@gmail.com>

hwrng: cctrng - cancel deferred works on unbind to avoid use-after-free

This commit replaces INIT_WORK() with devm_work_autocancel() for
compwork and startwork in cctrng_probe() so that pending work items are
canceled on driver unbind before drvdata is freed.

> diff --git a/drivers/char/hw_random/cctrng.c b/drivers/char/hw_random/cctrng.c
> index 03401ee59a8e8..138cc7072b023 100644
> --- a/drivers/char/hw_random/cctrng.c
> +++ b/drivers/char/hw_random/cctrng.c

[ ... ]

> @@ -497,8 +498,14 @@ static int cctrng_probe(struct platform_device *pdev)
>  		return dev_err_probe(dev, PTR_ERR(drvdata->clk),
>  				     "Failed to get or enable the clock\n");
>  
> -	INIT_WORK(&drvdata->compwork, cc_trng_compwork_handler);
> -	INIT_WORK(&drvdata->startwork, cc_trng_startwork_handler);
> +	rc = devm_work_autocancel(dev, &drvdata->compwork,
> +				  cc_trng_compwork_handler);
> +	if (rc)
> +		return rc;
> +	rc = devm_work_autocancel(dev, &drvdata->startwork,
> +				  cc_trng_startwork_handler);
> +	if (rc)
> +		return rc;
>  	spin_lock_init(&drvdata->read_lock);
>  
>  	/* register the driver isr function */

[Severity: High]
This problem wasn't introduced by this patch, but later in cctrng_probe(),
does cc_trng_pm_get() fail with -EACCES when CONFIG_PM is enabled [1]?

drivers/char/hw_random/cctrng.c:cctrng_probe() {
	...
	/* init PM */
	rc = cc_trng_pm_init(drvdata);
	if (rc)
		return dev_err_probe(dev, rc, "cc_trng_pm_init failed\n");

	/* increment device's usage counter */
	rc = cc_trng_pm_get(dev);
	if (rc)
		return dev_err_probe(dev, rc, "cc_trng_pm_get returned %x\n", rc);
	...
	/* All set, we can allow auto-suspend */
	cc_trng_pm_go(drvdata);
	...
}

cc_trng_pm_init() calls pm_runtime_set_active(), while pm_runtime_enable()
is deferred until cc_trng_pm_go() at the end of probe.  When
cc_trng_pm_get() calls pm_runtime_resume_and_get(), runtime PM is still
disabled and dev->power.last_status is RPM_INVALID, so rpm_resume()
returns -EACCES and cctrng_probe() aborts.

[1] https://sashiko.dev/#/bug/linux-6da7f352-ac7d-4b81-8569-a1008cfc3400

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