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

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

Pre-existing issues:
- [High] Unbalanced clock cleanup in the runtime PM resume error path
- [High] Memory leak and Use-After-Free of debugfs entries during device removal
- [High] Use-After-Free of device context when unbinding the device via sysfs with active TFMs
--

--- Patch [1]: [PATCH] crypto: sun4i-ss: fix autosuspend cleanup during teardown ---
commit 498a2e7e81cded38e9654e9d63d7f5c6261e2d92
Author: Guangshuo Li <[email protected]>

crypto: sun4i-ss: fix autosuspend cleanup during teardown

sun4i_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 sun4i_ss_pm_exit().

If the autosuspend delay is set to a negative value while autosuspend
is enabled, the runtime PM core increments usage_count to prevent
runtime suspend. Without calling pm_runtime_dont_use_autosuspend()
during cleanup, this reference is not dropped and usage_count remains
unbalanced.

Undo the autosuspend setting when pm_runtime_set_suspended() fails and
in sun4i_ss_pm_exit() before disabling runtime PM.

Fixes: 554c42b4efd4 ("crypto: sun4i-ss - enable pm_runtime")
Cc: [email protected]

> diff --git a/drivers/crypto/allwinner/sun4i-ss/sun4i-ss-core.c b/drivers/crypto/allwinner/sun4i-ss/sun4i-ss-core.c
> index 35ef0930e77f1..10c0f05994ed8 100644
> --- a/drivers/crypto/allwinner/sun4i-ss/sun4i-ss-core.c
> +++ b/drivers/crypto/allwinner/sun4i-ss/sun4i-ss-core.c
> @@ -304,14 +304,17 @@ static int sun4i_ss_pm_init(struct sun4i_ss_ctx *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 sun4i_ss_pm_exit(struct sun4i_ss_ctx *ss)
>  {
> +	pm_runtime_dont_use_autosuspend(ss->dev);
>  	pm_runtime_disable(ss->dev);
>  }

[Severity: High]
This is a pre-existing issue, but does the error path in sun4i_ss_pm_resume()
corrupt the clock framework's reference counting?

If clk_prepare_enable(ss->busclk) fails, the code jumps to err_enable:

err_enable:
    sun4i_ss_pm_suspend(dev);
    return err;

sun4i_ss_pm_suspend() then unconditionally disables both clocks:

    clk_disable_unprepare(ss->ssclk);
    clk_disable_unprepare(ss->busclk);

Since busclk failed to enable and ssclk wasn't attempted yet, does calling
clk_disable_unprepare() on them drop the prepare_count and enable_count below
zero?

[Severity: High]
This isn't a bug introduced by this patch, but does the driver leak debugfs
entries during device removal?

In sun4i_ss_probe(), the debugfs entries are created:

    ss->dbgfs_dir = debugfs_create_dir("sun4i-ss", NULL);
    ss->dbgfs_stats = debugfs_create_file("stats", 0444, ss->dbgfs_dir, ss,
                                          &sun4i_ss_debugfs_fops);

The device removal path in sun4i_ss_remove() lacks a corresponding
debugfs_remove_recursive() call.

Could this leak the debugfs dentries, and potentially lead to a use-after-free
if a user reads the leaked stats file after devres has freed the ss context?

[Severity: High]
This is a pre-existing issue, but is there a use-after-free risk when unbinding
the device with active TFMs?

In sun4i_ss_remove(), algorithms are unregistered:

    crypto_unregister_skcipher(&ss_algs[i].alg.crypto);

This merely removes the algorithms from the active list without waiting for
active TFMs to close. Since the context is managed by devm_kzalloc, devres
will free it immediately after remove completes.

If an unprivileged user keeps a TFM open, could callbacks like
sun4i_ss_cipher_poll() access freed memory when dereferencing op->ss?

sun4i_ss_cipher_poll()
    struct crypto_skcipher *tfm = crypto_skcipher_reqtfm(areq);
    struct sun4i_tfm_ctx *op = crypto_skcipher_ctx(tfm);
    // op->ss is accessed here

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1