Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
Pre-existing issues:
- [High] The debugfs directory and files created during probe are never removed on driver unbind, module unload, or probe failure, leading to a resource leak and potential Use-After-Free or page fault.
- [High] The driver modifies a static global array (`ss_algs`) to store per-device context (`ss`), which breaks if multiple instances of the device are probed.
- [High] Missing check on `sg_miter_next` return value leads to an infinite loop (DoS) inside a `spin_lock_bh` critical section.
--
commit 0aa56ed964b0d48c219e572488ad7609430a622a
Author: Eric Biggers <ebiggers@kernel.org>
crypto: sun4i-ss - Remove insecure and unused rng_alg
This commit removes the insecure and unused sun4i_ss_rng algorithm to
eliminate its multiple vulnerabilities, such as missing locking and buffer
overflows. The rng algorithm type of AF_ALG was never genuinely used by
userspace here anyway.
> diff --git a/drivers/crypto/allwinner/sun4i-ss/sun4i-ss-core.c b/drivers/crypto/allwinner/sun4i-ss/sun4i-ss-core.c
> --- a/drivers/crypto/allwinner/sun4i-ss/sun4i-ss-core.c
> +++ b/drivers/crypto/allwinner/sun4i-ss/sun4i-ss-core.c
> @@ -473,15 +448,6 @@ static int sun4i_ss_probe(struct platform_device *pdev)
> goto error_alg;
> }
> break;
> -#ifdef CONFIG_CRYPTO_DEV_SUN4I_SS_PRNG
> - 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;
> -#endif
> }
> }
[Severity: High]
This isn't a bug introduced by this patch, but does the algorithm
registration loop here overwrite the global device context?
The algorithm templates are defined in a static global array ss_algs. In
sun4i_ss_probe(), the driver loops over this array and assigns the
per-device context:
sun4i_ss_probe() {
...
for (i = 0; i < ARRAY_SIZE(ss_algs); i++) {
ss_algs[i].ss = ss;
...
}
If a system has multiple instances of this IP block, could the second probe
overwrite the ss pointers in the global struct, causing cryptographic
requests for the first device to incorrectly use the hardware context of
the second?
> @@ -526,11 +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;
> -#ifdef CONFIG_CRYPTO_DEV_SUN4I_SS_PRNG
> - case CRYPTO_ALG_TYPE_RNG:
> - crypto_unregister_rng(&ss_algs[i].alg.rng);
> - break;
> -#endif
> }
> }
>
[Severity: High]
This is a pre-existing issue, but does sun4i_ss_remove() fail to clean up
the debugfs entries?
In sun4i_ss_probe(), the driver creates a debugfs directory and a stats file
using debugfs_create_dir() and debugfs_create_file(), storing the ss context
as private data.
Neither sun4i_ss_remove() nor the error cleanup paths in sun4i_ss_probe()
appear to call debugfs_remove_recursive() to clean it up. If the module is
unloaded, could the debugfs file remain and cause a page fault if read
subsequently?
[Severity: High]
This is another pre-existing issue, but I noticed a potential infinite loop
in sun4i_hash() in drivers/crypto/allwinner/sun4i-ss/sun4i-ss-hash.c.
Does sun4i_hash() ignore the return value of sg_miter_next()?
sun4i_hash() {
...
while (op->len < 64 && i < end) {
...
if (in_i == mi.length) {
sg_miter_next(&mi);
in_i = 0;
}
...
}
If sg_miter_next() fails and returns false (e.g., when the scatterlist is
exhausted or malformed), mi.length is 0. Inside the loop, because no data is
processed, the loop counters never advance. The in_i == mi.length check
would repeatedly evaluate to 0 == 0, recursively issuing failing
sg_miter_next() calls.
Since this happens with bottom halves disabled via spin_lock_bh(&ss->slock),
could this spin forever and lead to a hard CPU lockup?
@@ -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
@@ -22,18 +22,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
@@ -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
@@ -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,18 +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;
-#ifdef CONFIG_CRYPTO_DEV_SUN4I_SS_PRNG
- 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;
-#endif
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);
@@ -471,19 +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;
-#ifdef CONFIG_CRYPTO_DEV_SUN4I_SS_PRNG
- 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;
-#endif
}
}
/* Ignore error of debugfs */
ss->dbgfs_dir = debugfs_create_dir("sun4i-ss", NULL);
@@ -499,15 +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;
-#ifdef CONFIG_CRYPTO_DEV_SUN4I_SS_PRNG
- case CRYPTO_ALG_TYPE_RNG:
- crypto_unregister_rng(&ss_algs[i].alg.rng);
- break;
-#endif
}
}
error_pm:
sun4i_ss_pm_exit(ss);
return err;
@@ -524,15 +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;
-#ifdef CONFIG_CRYPTO_DEV_SUN4I_SS_PRNG
- case CRYPTO_ALG_TYPE_RNG:
- crypto_unregister_rng(&ss_algs[i].alg.rng);
- break;
-#endif
}
}
sun4i_ss_pm_exit(ss);
}
deleted file mode 100644
@@ -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;
-}
@@ -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);