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

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

[Pengpeng Hou]

sun4i_ss_probe() creates a debugfs directory and a stats file with struct
sun4i_ss_ctx as private data. The remove path unregisters the crypto
algorithms and tears down runtime PM but leaves the debugfs entries
published.

Remove the debugfs subtree before tearing down the driver state used by
the stats show callback.

Signed-off-by: Pengpeng Hou <pengpeng@iscas.ac.cn>
---
 drivers/crypto/allwinner/sun4i-ss/sun4i-ss-core.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/crypto/allwinner/sun4i-ss/sun4i-ss-core.c b/drivers/crypto/allwinner/sun4i-ss/sun4i-ss-core.c
index 58a76e2ba64e..bcaddf1b83ca 100644
--- a/drivers/crypto/allwinner/sun4i-ss/sun4i-ss-core.c
+++ b/drivers/crypto/allwinner/sun4i-ss/sun4i-ss-core.c
@@ -512,6 +512,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);
+
 	for (i = 0; i < ARRAY_SIZE(ss_algs); i++) {
 		switch (ss_algs[i].type) {
 		case CRYPTO_ALG_TYPE_SKCIPHER:
-- 
2.50.1 (Apple Git-155)

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

[sashiko-bot]

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