Re: [PATCH] crypto: sun8i-ss: fix autosuspend cleanup during teardown

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but a kernel panic occurs due to a NULL pointer dereference when tearing down uninitialized flows on memory allocation failure.
- [High] This problem wasn't introduced by this patch, but a Use-After-Free of the device struct occurs on asynchronous sysfs device unbind when TFMs remain active.
- [High] This problem wasn't introduced by this patch, but the interrupt handler reads hardware registers without checking if the device is active, violating PM subsystem guidelines and risking a bus fault.
--

--- Patch [1]: [PATCH] crypto: sun8i-ss: fix autosuspend cleanup during teardown ---
commit 75226cf213617a07b13c36d7aa3f2f0e09bc4f0f
Author: Guangshuo Li <lgs201920130244@gmail.com>

crypto: sun8i-ss: fix autosuspend cleanup during teardown

sun8i_ss_pm_init() calls pm_runtime_use_autosuspend(), but the autosuspend
setting is not undone if pm_runtime_set_suspended() fails or when runtime PM
is later torn down by sun8i_ss_pm_exit(). This patch ensures the autosuspend
setting is correctly cleaned up by calling pm_runtime_dont_use_autosuspend().

> diff --git a/drivers/crypto/allwinner/sun8i-ss/sun8i-ss-core.c b/drivers/crypto/allwinner/sun8i-ss/sun8i-ss-core.c
> index 59c9bc45ec0f2..775355ac9f9e2 100644
> --- a/drivers/crypto/allwinner/sun8i-ss/sun8i-ss-core.c
> +++ b/drivers/crypto/allwinner/sun8i-ss/sun8i-ss-core.c
> @@ -664,14 +664,18 @@ static int sun8i_ss_pm_init(struct sun8i_ss_dev *ss)
>  	pm_runtime_set_autosuspend_delay(ss->dev, 2000);
>  
>  	err = pm_runtime_set_suspended(ss->dev);
> -	if (err)
> +	if (err) {
> +		pm_runtime_dont_use_autosuspend(ss->dev);
>  		return err;
> +	}
> +
>  	pm_runtime_enable(ss->dev);
>  	return err;
>  }
>  
>  static void sun8i_ss_pm_exit(struct sun8i_ss_dev *ss)
>  {
> +	pm_runtime_dont_use_autosuspend(ss->dev);
>  	pm_runtime_disable(ss->dev);
>  }

[Severity: High]
This isn't a bug introduced by this patch, but a kernel panic occurs due to
a NULL pointer dereference when tearing down uninitialized flows on memory
allocation failure.

If a memory allocation fails inside allocate_flows(), the code jumps to
error_engine without decrementing the loop index i:

drivers/crypto/allwinner/sun8i-ss/sun8i-ss-core.c:allocate_flows() {
    ...
		ss->flows[i].biv = devm_kmalloc(ss->dev, AES_BLOCK_SIZE,
						GFP_KERNEL);
		if (!ss->flows[i].biv) {
			err = -ENOMEM;
			goto error_engine;
		}
    ...
}

It then executes sun8i_ss_free_flows(ss, i). Because ss->flows was allocated
with devm_kcalloc (zero-initialized), ss->flows[i].engine is still NULL:

drivers/crypto/allwinner/sun8i-ss/sun8i-ss-core.c:sun8i_ss_free_flows() {
static void sun8i_ss_free_flows(struct sun8i_ss_dev *ss, int i)
{
	while (i >= 0) {
		crypto_engine_exit(ss->flows[i].engine);
		i--;
	}
}

Will this pass the NULL engine pointer to crypto_engine_exit() and cause a
crash when it is dereferenced?

[Severity: High]
This isn't a bug introduced by this patch, but a use-after-free of the
device struct occurs on asynchronous sysfs device unbind when TFMs remain
active.

During device unbind, sun8i_ss_remove() initiates the teardown without waiting
for active TFMs to be released, allowing devres to free the device structure
early:

drivers/crypto/allwinner/sun8i-ss/sun8i-ss-core.c:sun8i_ss_remove() {
static void sun8i_ss_remove(struct platform_device *pdev)
{
	struct sun8i_ss_dev *ss = platform_get_drvdata(pdev);

	sun8i_ss_unregister_algs(ss);
    ...
}

When the user process later closes its socket, does sun8i_ss_cipher_exit()
dereference the already freed op->ss structure?

drivers/crypto/allwinner/sun8i-ss/sun8i-ss-cipher.c:sun8i_ss_cipher_exit() {
void sun8i_ss_cipher_exit(struct crypto_tfm *tfm)
{
	struct sun8i_cipher_tfm_ctx *op = crypto_tfm_ctx(tfm);

	kfree_sensitive(op->key);
	crypto_free_skcipher(op->fallback_tfm);
	pm_runtime_put_sync(op->ss->dev);
}

[Severity: High]
This isn't a bug introduced by this patch, but the interrupt handler reads
hardware registers without checking if the device is active, violating PM
subsystem guidelines and risking a bus fault.

The interrupt handler immediately reads SS_INT_STA_REG upon entry:

drivers/crypto/allwinner/sun8i-ss/sun8i-ss-core.c:ss_irq_handler() {
static irqreturn_t ss_irq_handler(int irq, void *data)
{
	struct sun8i_ss_dev *ss = (struct sun8i_ss_dev *)data;
	int flow = 0;
	u32 p;

	p = readl(ss->base + SS_INT_STA_REG);
    ...
}

If a late or spurious interrupt fires while the device is in the RPM_SUSPENDED
state (with hardware clocks disabled), will this register read fault? Could
pm_runtime_get_if_active() be used before accessing hardware registers to
guarantee power is applied?

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