Re: [PATCH] crypto: sun4i-ss: remove debugfs directory on teardown

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Device unbind frees the devm-managed driver context while active crypto TFMs remain, leading to a Use-After-Free.
- [High] Global `ss_algs` array corruption upon multiple device probes or probe failures leaves dangling pointers, leading to a Use-After-Free.
--

commit ca25dde45b1d1ec5234ada4c67bcfb9e055331e7
Author: Pengpeng Hou <pengpeng@iscas.ac.cn>

crypto: sun4i-ss: remove debugfs directory on teardown

This commit addresses a cleanup omission in the sun4i-ss driver where the
debugfs directory and stats file were left published after the driver
was unbound. It adds a recursive debugfs removal during driver teardown.

> diff --git a/drivers/crypto/allwinner/sun4i-ss/sun4i-ss-core.c b/drivers/crypto/allwinner/sun4i-ss/sun4i-ss-core.c
> index 35ef0930e77f1..eadc5449b6807 100644
> --- a/drivers/crypto/allwinner/sun4i-ss/sun4i-ss-core.c
> +++ b/drivers/crypto/allwinner/sun4i-ss/sun4i-ss-core.c
> @@ -479,6 +479,8 @@ static void sun4i_ss_remove(struct platform_device *pdev)
>  	int i;
>  	struct sun4i_ss_ctx *ss = platform_get_drvdata(pdev);
>  
> +	debugfs_remove_recursive(ss->dbgfs_dir);
> +

[Severity: High]
This is a pre-existing issue, but does device unbind cause a use-after-free
here?

When the device is unbound via sysfs, sun4i_ss_remove() completes and devres
automatically frees the devm-managed ss context and unmaps ss->base.

However, unregistering the crypto algorithms only prevents new allocations.
It does not destroy existing active TFMs. If an active TFM continues to
operate, its context op->ss (which was copied from algt->ss during
sun4i_ss_cipher_init()) becomes a dangling pointer.

For example, a subsequent sun4i_ss_cipher_poll() call accessing the hardware:

drivers/crypto/allwinner/sun4i-ss/sun4i-ss-cipher.c:sun4i_ss_cipher_poll()
    ...
    writesl(ss->base + SS_RXFIFO, mi.addr + oi, todo);

will access unmapped memory and the freed ss context. Additionally, when the
TFM is eventually freed:

drivers/crypto/allwinner/sun4i-ss/sun4i-ss-cipher.c:sun4i_ss_cipher_exit()
    ...
    pm_runtime_put(op->ss->dev);

it dereferences the already-freed op->ss. Can this be addressed by tracking
active TFMs or ensuring they are quiesced before cleanup?

>  	for (i = 0; i < ARRAY_SIZE(ss_algs); i++) {
>  		switch (ss_algs[i].type) {
>  		case CRYPTO_ALG_TYPE_SKCIPHER:

[ ... ]

[Severity: High]
This is also a pre-existing issue, but is the driver context safely managed
if probe fails or if multiple devices are probed?

In sun4i_ss_probe(), the driver stores its per-device context pointer in the
statically allocated global array ss_algs:

drivers/crypto/allwinner/sun4i-ss/sun4i-ss-core.c:sun4i_ss_probe()
    for (i = 0; i < ARRAY_SIZE(ss_algs); i++) {
        ss_algs[i].ss = ss;

If a second device instance is probed, it overwrites ss_algs[0].ss with its
newly allocated ss context. The registration then fails with -EEXIST because
the first device already registered it, causing the second probe to abort
and devres to free its ss context.

However, ss_algs[0].ss remains pointing to this freed memory. When a new TFM
is created for the still-active first device:

drivers/crypto/allwinner/sun4i-ss/sun4i-ss-cipher.c:sun4i_ss_cipher_init()
    op->ss = algt->ss;

it copies the dangling pointer to op->ss, leading to a use-after-free when
accessing op->ss->dev. Could this global array be changed to avoid storing
per-device state?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260615091114.97190-1-pengpeng@iscas.ac.cn?part=1