linux-kernel - Re: [PATCH] hwrng: cctrng: Add cancel_work

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <Z1fRbqh2_2gSpgkJ@gondor.apana.org.au>
Date: Tue, 10 Dec 2024 13:28:14 +0800
From: Herbert Xu <herbert@...dor.apana.org.au>
To: Pei Xiao <xiaopei01@...inos.cn>
Cc: hadar.gat@....com, olivia@...enic.com, linux-crypto@...r.kernel.org,
	linux-kernel@...r.kernel.org, xiaopei01@...inos.cn,
	Tian Tao <tiantao6@...ilicon.com>
Subject: Re: [PATCH] hwrng: cctrng: Add cancel_work_sync before module remove

Pei Xiao <xiaopei01@...inos.cn> wrote:
> Be ensured that the work is canceled before proceeding with
> the cleanup in cc_trng_pm_fini.
> 
> Fixes: a583ed310bb6 ("hwrng: cctrng - introduce Arm CryptoCell driver")
> Signed-off-by: Pei Xiao <xiaopei01@...inos.cn>
> ---
> drivers/char/hw_random/cctrng.c | 2 ++
> 1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/char/hw_random/cctrng.c b/drivers/char/hw_random/cctrng.c
> index 4db198849695..fd1ee3687782 100644
> --- a/drivers/char/hw_random/cctrng.c
> +++ b/drivers/char/hw_random/cctrng.c
> @@ -127,6 +127,8 @@ static void cc_trng_pm_fini(struct cctrng_drvdata *drvdata)
> {
>        struct device *dev = &(drvdata->pdev->dev);
> 
> +       cancel_work_sync(&drvdata->compwork);
> +       cancel_work_sync(&drvdata->startwork);
>        pm_runtime_disable(dev);
> }

I don't think this is right.  This is a problem with using devres:
you need to be very careful with unwinding things.  The remove
occurs before all devm resources are released.  So the device is
still registered with the hwrng core and potentially live.

These work tasks are created by the ISR.  So they should only be
cancelled after the IRQ handler has been unregistered.

Perhaps we should just rip out all the devm code and go back to
manual freeing.

Cheers,
-- 
Email: Herbert Xu <herbert@...dor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt