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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] `cmh_rsa_set_pub_key()` fails to clear an existing private key, leading to decoupled key sizes and potential hardware out-of-bounds reads.
- [High] The driver does not validate the size of the RSA public exponent (`rsa.e_sz`), leading to potential hardware buffer overflows.
- [Medium] The RSA encrypt and decrypt functions return `-EINVAL` instead of `-EOVERFLOW` when the destination buffer is too small.
--

--- Patch [12]: [PATCH v2 12/19] crypto: cmh - add RSA akcipher ---
commit 47806773e523d39d4934a94df67997dcea3ac88f
Author: Alex Ousherovitch <aousherovitch@rambus.com>

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 multiple
key sizes, utilizing common PKE helpers shared across subsequent patches.

> diff --git a/drivers/crypto/cmh/cmh_pke_rsa.c b/drivers/crypto/cmh/cmh_pke_rsa.c
> --- /dev/null
> +++ b/drivers/crypto/cmh/cmh_pke_rsa.c

[ ... ]

> +static int cmh_rsa_enc(struct akcipher_request *req)
> +{
> +	struct crypto_akcipher *tfm = crypto_akcipher_reqtfm(req);
> +	struct cmh_rsa_tfm_ctx *ctx = cmh_rsa_ctx(tfm);
> +	struct cmh_rsa_reqctx *rctx = akcipher_request_ctx(req);
> +	u32 key_bytes = ctx->bits / 8;
> +	u32 e_padded = ALIGN(ctx->e_sz, 4);
> +	struct core_dispatch d = cmh_core_select_instance(CMH_CORE_PKE);
> +	struct vcq_cmd vcq[PKE_VCQ_CMDS_MIN];
> +	int ret, nents;
> +	gfp_t gfp;
> +
> +	if (!ctx->n || !ctx->e)
> +		return -EINVAL;
> +	if (req->src_len > key_bytes || req->dst_len < key_bytes)
> +		return -EINVAL;

[Severity: Medium]
Does this violate the akcipher API contract by returning -EINVAL instead of
-EOVERFLOW? If the provided destination buffer is too small (req->dst_len <
key_bytes), returning -EOVERFLOW and updating req->dst_len to the required
size allows callers like af_alg to dynamically reallocate memory and retry.

[ ... ]

> +static int cmh_rsa_dec(struct akcipher_request *req)
> +{
> +	struct crypto_akcipher *tfm = crypto_akcipher_reqtfm(req);
> +	struct cmh_rsa_tfm_ctx *ctx = cmh_rsa_ctx(tfm);
> +	struct cmh_rsa_reqctx *rctx = akcipher_request_ctx(req);
> +	u32 key_bytes = ctx->bits / 8;
> +	u32 e_padded = ALIGN(ctx->e_sz, 4);
> +	struct vcq_cmd vcq[PKE_VCQ_CMDS_MAX];
> +	struct core_dispatch dd;
> +	int ret, idx, nents;
> +	gfp_t gfp;
> +
> +	if (ctx->key.mode != CMH_KEY_RAW)
> +		return -EINVAL;
> +	if (!ctx->n || !ctx->e)
> +		return -EINVAL;
> +	if (req->src_len > key_bytes || req->dst_len < key_bytes)
> +		return -EINVAL;

[Severity: Medium]
Similarly, should this decryption path return -EOVERFLOW and update
req->dst_len when the buffer is insufficient, allowing userspace applications
to properly query the required buffer size?

[ ... ]

> +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;
> +
> +	ret = rsa_parse_pub_key(&rsa, key, keylen);
> +	if (ret)
> +		return 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;
> +
> +	kfree(ctx->n);
> +	kfree(ctx->e);
> +	ctx->n = NULL;
> +	ctx->e = NULL;
> +	ctx->n_sz = 0;
> +	ctx->e_sz = 0;

[Severity: High]
Could this lead to a state mismatch by failing to clear an existing private
key? If a small private key is loaded followed by a large public key,
cmh_rsa_dec() might later write a short private key into SYS_REF_TEMP while
issuing PKE_CMD_RSA_DEC configured for a much larger key size. Would calling
cmh_key_destroy(&ctx->key) here prevent this potential out-of-bounds read by
the hardware?

> +
> +	ctx->n = kmemdup(rsa.n, rsa.n_sz, GFP_KERNEL);
> +	ctx->e = kmemdup(rsa.e, rsa.e_sz, GFP_KERNEL);

[Severity: High]
Is there a missing upper bound check for the RSA public exponent size
(rsa.e_sz) here? An attacker could supply an oversized exponent in the ASN.1
key blob, causing the driver to allocate a large buffer and pass its length
to the hardware command queue in cmh_rsa_enc(). Could this trigger a buffer
overflow in the hardware crypto core's SRAM?

[ ... ]

> +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;
> +
> +	ret = rsa_parse_priv_key(&rsa, key, keylen);
> +	if (ret)
> +		return ret;
> +
[ ... ]
> +	kfree(ctx->n);
> +	kfree(ctx->e);
> +	ctx->n = NULL;
> +	ctx->e = NULL;
> +	ctx->n_sz = 0;
> +	ctx->e_sz = 0;
> +
> +	ctx->n = kmemdup(rsa.n, rsa.n_sz, GFP_KERNEL);
> +	ctx->e = kmemdup(rsa.e, rsa.e_sz, GFP_KERNEL);

[Severity: High]
As in cmh_rsa_set_pub_key(), does this unvalidated copy of rsa.e_sz risk
crashing the hardware if a malformed private key with a massive public
exponent is provided?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260709203037.1884436-1-skrishnamoorthy@rambus.com?part=12