Re: [PATCH v2] crypto: sun4i-ss - Remove insecure and unused rng_alg

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

Pre-existing issues:
- [High] Missing debugfs cleanup in sun4i_ss_remove() causes a use-after-free when debugfs files are accessed after module unload.
- [High] Device unbind causes a use-after-free of the devm-allocated device context via active crypto transforms.
- [High] Incorrect error handling in sun4i_ss_pm_resume() causes clock reference count underflows.
--

commit 678d50ec6e8d7e2858de6d7ee66a1f1a63b0e288
Author: Eric Biggers <ebiggers@kernel.org>

crypto: sun4i-ss - Remove insecure and unused rng_alg

This commit removes the unused and insecure `sun4i_ss_rng` algorithm from the
sun4i-ss crypto driver. It had multiple vulnerabilities including missing
locking, a buffer overflow in `sun4i_ss_prng_seed()`, and insufficient
random byte generation.

Closes: https://lore.kernel.org/r/af749a8447bd7f0e9dd26ca6c87e9c6afecb09d9@linux.dev/

> diff --git a/drivers/crypto/allwinner/sun4i-ss/sun4i-ss-core.c b/drivers/crypto/allwinner/sun4i-ss/sun4i-ss-core.c
> index 58a76e2ba64e2..35ef0930e77f1 100644
> --- a/drivers/crypto/allwinner/sun4i-ss/sun4i-ss-core.c
> +++ b/drivers/crypto/allwinner/sun4i-ss/sun4i-ss-core.c

[ ... ]

> @@ -520,9 +487,6 @@ static void sun4i_ss_remove(struct platform_device *pdev)
>  		case CRYPTO_ALG_TYPE_AHASH:
>  			crypto_unregister_ahash(&ss_algs[i].alg.hash);
>  			break;

[Severity: High]
This isn't a bug introduced by this patch, but does the device unbind cause a
use-after-free of the devm-allocated device context here?

The device context `ss` is allocated via `devm_kzalloc()`. If the device is
unbound via sysfs, `sun4i_ss_remove()` runs and returns, triggering `devm`
to free `ss` and unmap the hardware MMIO region.

However, `crypto_unregister_alg()` does not wait for existing active crypto
transforms (`tfm`) to be closed. Any subsequent encryption requests or
calls to `sun4i_ss_cipher_exit()` on these surviving `tfm`s will dereference
the freed `ss` pointer (`op->ss->dev`) and attempt to access unmapped memory.

> -		case CRYPTO_ALG_TYPE_RNG:
> -			crypto_unregister_rng(&ss_algs[i].alg.rng);
> -			break;
>  		}
>  	}
>  

[Severity: High]
This is a pre-existing issue, but are we missing a debugfs cleanup before
this function returns?

The driver creates a debugfs directory and a `stats` file in
`sun4i_ss_probe()` but never removes them here in `sun4i_ss_remove()`.

Because the debugfs entries are not cleaned up, unloading the module leaves
stale dentry/inodes. If a user subsequently accesses the
`/sys/kernel/debug/sun4i-ss/stats` file, the VFS will attempt to execute
unmapped memory (dangling `sun4i_ss_debugfs_fops`), leading to a kernel
Oops or possible privilege escalation.


[Severity: High]
This is also a pre-existing issue, but does incorrect error handling in
`sun4i_ss_pm_resume()` cause clock reference count underflows?

Looking at `sun4i_ss_pm_resume()`:

drivers/crypto/allwinner/sun4i-ss/sun4i-ss-core.c:sun4i_ss_pm_resume() {
    ...
err_enable:
	sun4i_ss_pm_suspend(dev);
	return err;
}

If `clk_prepare_enable()` fails for either `busclk` or `ssclk`, the error
path jumps to `err_enable` and unconditionally calls `sun4i_ss_pm_suspend()`.

The suspend function calls `clk_disable_unprepare()` on both clocks, meaning
it will disable a clock that failed to enable. This causes a reference count
underflow in the Common Clock Framework, potentially disabling shared clocks
(`busclk` is the AHB bus clock) while other devices are actively using them.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260601160757.79645-1-ebiggers@kernel.org?part=1