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

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

[Eric Biggers]

Remove sun4i_ss_rng, as it is insecure and unused:

- It has multiple vulnerabilities.  sun4i_ss_prng_seed() is missing
  locking and has a buffer overflow.  sun4i_ss_prng_generate() fails to
  fill the entire buffer with cryptographic random bytes, because it
  rounds the destination length down and also doesn't actually wait for
  the hardware to be ready before pulling bytes from it.

- No user of this code is known.  It's usable only theoretically via the
  "rng" algorithm type of AF_ALG.  But userspace actually just uses the
  actual Linux RNG (/dev/random etc) instead.  And rng_algs don't
  contribute entropy to the actual Linux RNG either.  (This may have
  been confused with hwrng, which does contribute entropy.)

The sun4i_ss_prng_seed() buffer overflow was reported by Tianchu Chen
and discovered by Atuin - Automated Vulnerability Discovery Engine

There's no point in fixing all these vulnerabilities individually when
this is unused code, so let's just remove it.

Fixes: b8ae5c7387ad ("crypto: sun4i-ss - support the Security System PRNG")
Cc: stable@vger.kernel.org
Reported-by: Tianchu Chen <flynnnchen@tencent.com>
Closes: https://lore.kernel.org/r/af749a8447bd7f0e9dd26ca6c87e9c6afecb09d9@linux.dev/
Acked-by: Corentin LABBE <clabbe.montjoie@gmail.com>
Signed-off-by: Eric Biggers <ebiggers@kernel.org>
---

This patch is targeting crypto/master

v2: rebased onto crypto/master, and added Acked-by and Reported-by

 arch/arm/configs/sunxi_defconfig              |  1 -
 drivers/crypto/allwinner/Kconfig              |  8 ---
 drivers/crypto/allwinner/sun4i-ss/Makefile    |  1 -
 .../crypto/allwinner/sun4i-ss/sun4i-ss-core.c | 36 ----------
 .../crypto/allwinner/sun4i-ss/sun4i-ss-prng.c | 69 -------------------
 drivers/crypto/allwinner/sun4i-ss/sun4i-ss.h  | 20 ------
 6 files changed, 135 deletions(-)
 delete mode 100644 drivers/crypto/allwinner/sun4i-ss/sun4i-ss-prng.c

diff --git a/arch/arm/configs/sunxi_defconfig b/arch/arm/configs/sunxi_defconfig
index a83d29fed175..f4b8d8f7dbef 100644
--- a/arch/arm/configs/sunxi_defconfig
+++ b/arch/arm/configs/sunxi_defconfig
@@ -168,11 +168,10 @@ CONFIG_NFS_V3_ACL=y
 CONFIG_NFS_V4=y
 CONFIG_ROOT_NFS=y
 CONFIG_NLS_CODEPAGE_437=y
 CONFIG_NLS_ISO8859_1=y
 CONFIG_CRYPTO_DEV_SUN4I_SS=y
-CONFIG_CRYPTO_DEV_SUN4I_SS_PRNG=y
 CONFIG_CRYPTO_DEV_SUN8I_CE=y
 CONFIG_CRYPTO_DEV_SUN8I_SS=y
 CONFIG_DMA_CMA=y
 CONFIG_PRINTK_TIME=y
 CONFIG_DEBUG_FS=y
diff --git a/drivers/crypto/allwinner/Kconfig b/drivers/crypto/allwinner/Kconfig
index 7270e5fbc573..1048f8e95ba8 100644
--- a/drivers/crypto/allwinner/Kconfig
+++ b/drivers/crypto/allwinner/Kconfig
@@ -23,18 +23,10 @@ config CRYPTO_DEV_SUN4I_SS
 	  and SHA1 and MD5 hash algorithms.
 
 	  To compile this driver as a module, choose M here: the module
 	  will be called sun4i-ss.
 
-config CRYPTO_DEV_SUN4I_SS_PRNG
-	bool "Support for Allwinner Security System PRNG"
-	depends on CRYPTO_DEV_SUN4I_SS
-	select CRYPTO_RNG
-	help
-	  Select this option if you want to provide kernel-side support for
-	  the Pseudo-Random Number Generator found in the Security System.
-
 config CRYPTO_DEV_SUN4I_SS_DEBUG
 	bool "Enable sun4i-ss stats"
 	depends on CRYPTO_DEV_SUN4I_SS
 	depends on DEBUG_FS
 	help
diff --git a/drivers/crypto/allwinner/sun4i-ss/Makefile b/drivers/crypto/allwinner/sun4i-ss/Makefile
index c0a2797d3168..06a9ae81f9f8 100644
--- a/drivers/crypto/allwinner/sun4i-ss/Makefile
+++ b/drivers/crypto/allwinner/sun4i-ss/Makefile
@@ -1,4 +1,3 @@
 # SPDX-License-Identifier: GPL-2.0-only
 obj-$(CONFIG_CRYPTO_DEV_SUN4I_SS) += sun4i-ss.o
 sun4i-ss-y += sun4i-ss-core.o sun4i-ss-hash.o sun4i-ss-cipher.o
-sun4i-ss-$(CONFIG_CRYPTO_DEV_SUN4I_SS_PRNG) += sun4i-ss-prng.o
diff --git a/drivers/crypto/allwinner/sun4i-ss/sun4i-ss-core.c b/drivers/crypto/allwinner/sun4i-ss/sun4i-ss-core.c
index 58a76e2ba64e..35ef0930e77f 100644
--- a/drivers/crypto/allwinner/sun4i-ss/sun4i-ss-core.c
+++ b/drivers/crypto/allwinner/sun4i-ss/sun4i-ss-core.c
@@ -211,27 +211,10 @@ static struct sun4i_ss_alg_template ss_algs[] = {
 			.cra_init = sun4i_ss_cipher_init,
 			.cra_exit = sun4i_ss_cipher_exit,
 		}
 	}
 },
-#ifdef CONFIG_CRYPTO_DEV_SUN4I_SS_PRNG
-{
-	.type = CRYPTO_ALG_TYPE_RNG,
-	.alg.rng = {
-		.base = {
-			.cra_name		= "stdrng",
-			.cra_driver_name	= "sun4i_ss_rng",
-			.cra_priority		= 300,
-			.cra_ctxsize		= 0,
-			.cra_module		= THIS_MODULE,
-		},
-		.generate               = sun4i_ss_prng_generate,
-		.seed                   = sun4i_ss_prng_seed,
-		.seedsize               = SS_SEED_LEN / BITS_PER_BYTE,
-	}
-},
-#endif
 };
 
 static int sun4i_ss_debugfs_show(struct seq_file *seq, void *v)
 {
 	unsigned int i;
@@ -245,16 +228,10 @@ static int sun4i_ss_debugfs_show(struct seq_file *seq, void *v)
 				   ss_algs[i].alg.crypto.base.cra_driver_name,
 				   ss_algs[i].alg.crypto.base.cra_name,
 				   ss_algs[i].stat_req, ss_algs[i].stat_opti, ss_algs[i].stat_fb,
 				   ss_algs[i].stat_bytes);
 			break;
-		case CRYPTO_ALG_TYPE_RNG:
-			seq_printf(seq, "%s %s reqs=%lu tsize=%lu\n",
-				   ss_algs[i].alg.rng.base.cra_driver_name,
-				   ss_algs[i].alg.rng.base.cra_name,
-				   ss_algs[i].stat_req, ss_algs[i].stat_bytes);
-			break;
 		case CRYPTO_ALG_TYPE_AHASH:
 			seq_printf(seq, "%s %s reqs=%lu\n",
 				   ss_algs[i].alg.hash.halg.base.cra_driver_name,
 				   ss_algs[i].alg.hash.halg.base.cra_name,
 				   ss_algs[i].stat_req);
@@ -469,17 +446,10 @@ static int sun4i_ss_probe(struct platform_device *pdev)
 				dev_err(ss->dev, "Fail to register %s\n",
 					ss_algs[i].alg.hash.halg.base.cra_name);
 				goto error_alg;
 			}
 			break;
-		case CRYPTO_ALG_TYPE_RNG:
-			err = crypto_register_rng(&ss_algs[i].alg.rng);
-			if (err) {
-				dev_err(ss->dev, "Fail to register %s\n",
-					ss_algs[i].alg.rng.base.cra_name);
-			}
-			break;
 		}
 	}
 
 	/* Ignore error of debugfs */
 	ss->dbgfs_dir = debugfs_create_dir("sun4i-ss", NULL);
@@ -495,13 +465,10 @@ static int sun4i_ss_probe(struct platform_device *pdev)
 			crypto_unregister_skcipher(&ss_algs[i].alg.crypto);
 			break;
 		case CRYPTO_ALG_TYPE_AHASH:
 			crypto_unregister_ahash(&ss_algs[i].alg.hash);
 			break;
-		case CRYPTO_ALG_TYPE_RNG:
-			crypto_unregister_rng(&ss_algs[i].alg.rng);
-			break;
 		}
 	}
 error_pm:
 	sun4i_ss_pm_exit(ss);
 	return err;
@@ -518,13 +485,10 @@ static void sun4i_ss_remove(struct platform_device *pdev)
 			crypto_unregister_skcipher(&ss_algs[i].alg.crypto);
 			break;
 		case CRYPTO_ALG_TYPE_AHASH:
 			crypto_unregister_ahash(&ss_algs[i].alg.hash);
 			break;
-		case CRYPTO_ALG_TYPE_RNG:
-			crypto_unregister_rng(&ss_algs[i].alg.rng);
-			break;
 		}
 	}
 
 	sun4i_ss_pm_exit(ss);
 }
diff --git a/drivers/crypto/allwinner/sun4i-ss/sun4i-ss-prng.c b/drivers/crypto/allwinner/sun4i-ss/sun4i-ss-prng.c
deleted file mode 100644
index 491fcb7b81b4..000000000000
--- a/drivers/crypto/allwinner/sun4i-ss/sun4i-ss-prng.c
+++ /dev/null
@@ -1,69 +0,0 @@
-// SPDX-License-Identifier: GPL-2.0-or-later
-#include "sun4i-ss.h"
-
-int sun4i_ss_prng_seed(struct crypto_rng *tfm, const u8 *seed,
-		       unsigned int slen)
-{
-	struct sun4i_ss_alg_template *algt;
-	struct rng_alg *alg = crypto_rng_alg(tfm);
-
-	algt = container_of(alg, struct sun4i_ss_alg_template, alg.rng);
-	memcpy(algt->ss->seed, seed, slen);
-
-	return 0;
-}
-
-int sun4i_ss_prng_generate(struct crypto_rng *tfm, const u8 *src,
-			   unsigned int slen, u8 *dst, unsigned int dlen)
-{
-	struct sun4i_ss_alg_template *algt;
-	struct rng_alg *alg = crypto_rng_alg(tfm);
-	int i, err;
-	u32 v;
-	u32 *data = (u32 *)dst;
-	const u32 mode = SS_OP_PRNG | SS_PRNG_CONTINUE | SS_ENABLED;
-	size_t len;
-	struct sun4i_ss_ctx *ss;
-	unsigned int todo = (dlen / 4) * 4;
-
-	algt = container_of(alg, struct sun4i_ss_alg_template, alg.rng);
-	ss = algt->ss;
-
-	err = pm_runtime_resume_and_get(ss->dev);
-	if (err < 0)
-		return err;
-
-	if (IS_ENABLED(CONFIG_CRYPTO_DEV_SUN4I_SS_DEBUG)) {
-		algt->stat_req++;
-		algt->stat_bytes += todo;
-	}
-
-	spin_lock_bh(&ss->slock);
-
-	writel(mode, ss->base + SS_CTL);
-
-	while (todo > 0) {
-		/* write the seed */
-		for (i = 0; i < SS_SEED_LEN / BITS_PER_LONG; i++)
-			writel(ss->seed[i], ss->base + SS_KEY0 + i * 4);
-
-		/* Read the random data */
-		len = min_t(size_t, SS_DATA_LEN / BITS_PER_BYTE, todo);
-		readsl(ss->base + SS_TXFIFO, data, len / 4);
-		data += len / 4;
-		todo -= len;
-
-		/* Update the seed */
-		for (i = 0; i < SS_SEED_LEN / BITS_PER_LONG; i++) {
-			v = readl(ss->base + SS_KEY0 + i * 4);
-			ss->seed[i] = v;
-		}
-	}
-
-	writel(0, ss->base + SS_CTL);
-	spin_unlock_bh(&ss->slock);
-
-	pm_runtime_put(ss->dev);
-
-	return 0;
-}
diff --git a/drivers/crypto/allwinner/sun4i-ss/sun4i-ss.h b/drivers/crypto/allwinner/sun4i-ss/sun4i-ss.h
index 6c5d4aa6453c..f7d1c79ac677 100644
--- a/drivers/crypto/allwinner/sun4i-ss/sun4i-ss.h
+++ b/drivers/crypto/allwinner/sun4i-ss/sun4i-ss.h
@@ -29,12 +29,10 @@
 #include <crypto/hash.h>
 #include <crypto/internal/hash.h>
 #include <crypto/internal/skcipher.h>
 #include <crypto/aes.h>
 #include <crypto/internal/des.h>
-#include <crypto/internal/rng.h>
-#include <crypto/rng.h>
 
 #define SS_CTL            0x00
 #define SS_KEY0           0x04
 #define SS_KEY1           0x08
 #define SS_KEY2           0x0C
@@ -60,14 +58,10 @@
 #define SS_RXFIFO         0x200
 #define SS_TXFIFO         0x204
 
 /* SS_CTL configuration values */
 
-/* PRNG generator mode - bit 15 */
-#define SS_PRNG_ONESHOT		(0 << 15)
-#define SS_PRNG_CONTINUE	(1 << 15)
-
 /* IV mode for hash */
 #define SS_IV_ARBITRARY		(1 << 14)
 
 /* SS operation mode - bits 12-13 */
 #define SS_ECB			(0 << 12)
@@ -92,18 +86,14 @@
 #define SS_OP_AES		(0 << 4)
 #define SS_OP_DES		(1 << 4)
 #define SS_OP_3DES		(2 << 4)
 #define SS_OP_SHA1		(3 << 4)
 #define SS_OP_MD5		(4 << 4)
-#define SS_OP_PRNG		(5 << 4)
 
 /* Data end bit - bit 2 */
 #define SS_DATA_END		(1 << 2)
 
-/* PRNG start bit - bit 1 */
-#define SS_PRNG_START		(1 << 1)
-
 /* SS Enable bit - bit 0 */
 #define SS_DISABLED		(0 << 0)
 #define SS_ENABLED		(1 << 0)
 
 /* SS_FCSR configuration values */
@@ -126,13 +116,10 @@
 #define SS_RXFIFO_EMP_INT_PENDING	(1 << 10)
 #define SS_TXFIFO_AVA_INT_PENDING	(1 << 8)
 #define SS_RXFIFO_EMP_INT_ENABLE	(1 << 2)
 #define SS_TXFIFO_AVA_INT_ENABLE	(1 << 0)
 
-#define SS_SEED_LEN 192
-#define SS_DATA_LEN 160
-
 /*
  * struct ss_variant - Describe SS hardware variant
  * @sha1_in_be:		The SHA1 digest is given by SS in BE, and so need to be inverted.
  */
 struct ss_variant {
@@ -149,24 +136,20 @@ struct sun4i_ss_ctx {
 	struct device *dev;
 	struct resource *res;
 	char buf[4 * SS_RX_MAX];/* buffer for linearize SG src */
 	char bufo[4 * SS_TX_MAX]; /* buffer for linearize SG dst */
 	spinlock_t slock; /* control the use of the device */
-#ifdef CONFIG_CRYPTO_DEV_SUN4I_SS_PRNG
-	u32 seed[SS_SEED_LEN / BITS_PER_LONG];
-#endif
 	struct dentry *dbgfs_dir;
 	struct dentry *dbgfs_stats;
 };
 
 struct sun4i_ss_alg_template {
 	u32 type;
 	u32 mode;
 	union {
 		struct skcipher_alg crypto;
 		struct ahash_alg hash;
-		struct rng_alg rng;
 	} alg;
 	struct sun4i_ss_ctx *ss;
 	unsigned long stat_req;
 	unsigned long stat_fb;
 	unsigned long stat_bytes;
@@ -229,8 +212,5 @@ int sun4i_ss_aes_setkey(struct crypto_skcipher *tfm, const u8 *key,
 			unsigned int keylen);
 int sun4i_ss_des_setkey(struct crypto_skcipher *tfm, const u8 *key,
 			unsigned int keylen);
 int sun4i_ss_des3_setkey(struct crypto_skcipher *tfm, const u8 *key,
 			 unsigned int keylen);
-int sun4i_ss_prng_generate(struct crypto_rng *tfm, const u8 *src,
-			   unsigned int slen, u8 *dst, unsigned int dlen);
-int sun4i_ss_prng_seed(struct crypto_rng *tfm, const u8 *seed, unsigned int slen);

base-commit: ecf3edd349dfabee9bc8a46c5ff91c9ebd858d48
-- 
2.54.0.1013.g208068f2d8-goog

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

[sashiko-bot]

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