Re: [PATCH v3 12/19] crypto: cmh - add RSA akcipher

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Critical] State corruption in RSA akcipher key setup allows unprivileged kernel panic or out-of-bounds DMA access.
--

--- Patch [12]: [PATCH v3 12/19] crypto: cmh - add RSA akcipher ---
commit 1cce85c01c65473c016e5f55fbec7d241cfe672c
Author: Alex Ousherovitch <[email protected]>

crypto: cmh - add RSA akcipher

This commit registers the RSA akcipher algorithm using the CMH PKE core.
It supports encrypt, decrypt, sign, and verify operations with 2048, 3072, 
and 4096-bit keys, as well as 512 and 1024-bit keys for interoperability.

> diff --git a/drivers/crypto/cmh/cmh_pke_rsa.c b/drivers/crypto/cmh/cmh_pke_rsa.c
> new file mode 100644
> index 0000000000000..a848b7eddc8e2
> --- /dev/null
> +++ b/drivers/crypto/cmh/cmh_pke_rsa.c
[ ... ]
> +static int cmh_rsa_set_pub_key(struct crypto_akcipher *tfm,
> +			       const void *key, unsigned int keylen)
> +{
> +	struct cmh_rsa_tfm_ctx *ctx = cmh_rsa_ctx(tfm);
> +	struct rsa_key rsa = {};
> +	int ret;
[ ... ]
> +	/* Strip ASN.1 leading zero padding from modulus */
> +	while (rsa.n_sz > 0 && rsa.n[0] == 0) {
> +		rsa.n++;
> +		rsa.n_sz--;
> +	}
> +
> +	ctx->bits = cmh_rsa_key_bits(rsa.n_sz);
> +	if (!ctx->bits)
> +		return -EINVAL;
> +
> +	/* Reject an exponent wider than the modulus (HW buffer bound). */
> +	if (!rsa.e_sz || rsa.e_sz > ctx->bits / 8)
> +		return -EINVAL;

[Severity: Critical]
Does this code corrupt the tfm state if validation fails?

If a valid key is set first, ctx->n and ctx->e are populated. Then, if
a subsequent invalid key is provided (e.g. an unsupported modulus size where
cmh_rsa_key_bits() returns 0), ctx->bits is updated to 0 and an error is
returned.

This leaves the tfm in a split state where ctx->n and ctx->e are non-NULL
but ctx->bits is 0.

If a subsequent cmh_rsa_enc() is called with a zero-length source buffer:

cmh_rsa_enc()
   u32 key_bytes = ctx->bits / 8; // 0
   ...
   rctx->m_buf = kzalloc(key_bytes, gfp); // Returns ZERO_SIZE_PTR
   ...
   rctx->m_dma = cmh_dma_map_single(rctx->m_buf, key_bytes, DMA_TO_DEVICE);

The ZERO_SIZE_PTR is passed to cmh_dma_map_single(), which can cause a
kernel panic (Oops). This path appears reachable by unprivileged userspace
via the AF_ALG interface.

[ ... ]
> +static int cmh_rsa_set_priv_key(struct crypto_akcipher *tfm,
> +				const void *key, unsigned int keylen)
> +{
> +	struct cmh_rsa_tfm_ctx *ctx = cmh_rsa_ctx(tfm);
> +	struct rsa_key rsa = {};
> +	u32 key_bytes;
> +	u8 *d_padded;
> +	int ret;
[ ... ]
> +	/* Strip ASN.1 leading zero padding from modulus */
> +	while (rsa.n_sz > 0 && rsa.n[0] == 0) {
> +		rsa.n++;
> +		rsa.n_sz--;
> +	}
> +
> +	ctx->bits = cmh_rsa_key_bits(rsa.n_sz);
> +	if (!ctx->bits || !rsa.d_sz)
> +		return -EINVAL;
> +
> +	key_bytes = ctx->bits / 8;
> +
> +	/* Strip ASN.1 leading zero padding from private exponent */
> +	while (rsa.d_sz > 0 && rsa.d[0] == 0) {
> +		rsa.d++;
> +		rsa.d_sz--;
> +	}
> +
> +	if (!rsa.d_sz || rsa.d_sz > key_bytes)
> +		return -EINVAL;

[Severity: Critical]
Could this similarly corrupt ctx->bits on failure?

As with cmh_rsa_set_pub_key(), updating ctx->bits before completing all
validation checks allows the tfm to be left in an inconsistent state if an
error is returned, potentially leading to the same zero-size allocation and
DMA mapping crash during later operations.

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