Re: [PATCH v2 05/19] crypto: cmh - add HMAC ahash

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] The HMAC driver buffers unbounded input data in software, violating `ahash` API contracts for arbitrary data lengths and state export.
- [High] Dynamically allocated chunks leak when an ahash request is aborted or reused, allowing an unprivileged local DoS.
- [High] Unconditional use of `GFP_KERNEL` in `cmh_hmac_import` can cause the kernel to sleep in an atomic context.
- [High] Mismatched Core IDs between HMAC Key setup and HMAC operation execution.
--

--- Patch [5]: [PATCH v2 05/19] crypto: cmh - add HMAC ahash ---
commit dc6fee6be6ca3f7c1ff3752ad3551a1e1e242ea0
Author: Alex Ousherovitch <aousherovitch@rambus.com>

crypto: cmh - add HMAC ahash

Register ahash algorithms for HMAC-SHA-224, HMAC-SHA-256,
HMAC-SHA-384, HMAC-SHA-512, HMAC-SHA3-224, HMAC-SHA3-256,
HMAC-SHA3-384, and HMAC-SHA3-512 using the CMH hash core.

> diff --git a/drivers/crypto/cmh/cmh_hmac.c b/drivers/crypto/cmh/cmh_hmac.c
> new file mode 100644
> index 0000000000000..1f536088eabf2
> --- /dev/null
> +++ b/drivers/crypto/cmh/cmh_hmac.c
[ ... ]
> @@ -262,18 +262,14 @@ static int cmh_hmac_setkey(struct crypto_ahash *tfm, const u8 *key,
>  			   unsigned int keylen)
>  {
>  	struct cmh_hmac_tfm_ctx *tctx = crypto_ahash_ctx(tfm);
>  
>  	return cmh_key_setkey_raw(&tctx->key, key, keylen, CORE_ID_HC);

[Severity: High]
Is the core ID statically hardcoded to CORE_ID_HC here during key setup?

>  }
>  
>  static int cmh_hmac_init(struct ahash_request *req)
>  {
>  	struct crypto_ahash *tfm = crypto_ahash_reqtfm(req);
>  	struct cmh_hmac_reqctx *rctx = ahash_request_ctx(req);
>  
>  	rctx->info = cmh_hmac_get_info(tfm);
>  	rctx->error = 0;
>  	INIT_LIST_HEAD(&rctx->chunks);

[Severity: High]
If a request is reused (for example, by calling crypto_ahash_init() before
final()), will this blindly overwrite the chunk list and strand any previously
accumulated chunks?

>  	rctx->num_chunks = 0;
>  	rctx->total_len = 0;
>  
>  	return 0;
>  }
>  
>  static int cmh_hmac_update(struct ahash_request *req)
>  {
>  	struct crypto_ahash *tfm = crypto_ahash_reqtfm(req);
>  	struct cmh_hmac_tfm_ctx *tctx = crypto_ahash_ctx(tfm);
>  	struct cmh_hmac_reqctx *rctx = ahash_request_ctx(req);
>  	struct cmh_hmac_chunk *chunk;
>  	int nents;
>  
>  	if (rctx->error)
>  		return rctx->error;
>  
>  	if (!req->nbytes)
>  		return 0;
>  
>  	if (req->nbytes > HMAC_MAX_DATA - rctx->total_len) {

[Severity: High]
Does this code violate the ahash API contract by arbitrarily capping the
input data length at 64KB?

The crypto API expects ahash implementations to handle streaming inputs of
arbitrary lengths. Should we be allocating a software fallback (like shash)
to handle inputs larger than what the hardware can support?

>  		rctx->error = -EINVAL;
>  		goto err_free_chunks;
>  	}
[ ... ]
>  	list_add_tail(&chunk->list, &rctx->chunks);
>  	spin_lock_bh(&tctx->chunk_lock);
>  	list_add_tail(&chunk->tfm_node, &tctx->all_chunks);

[Severity: High]
The chunks are added to the TFM-wide list here, but if the request doesn't
complete, they will be left indefinitely on this list.

>  	spin_unlock_bh(&tctx->chunk_lock);
>  	rctx->num_chunks++;
>  	rctx->total_len += req->nbytes;
>  
>  	return 0;
[ ... ]
> @@ -428,14 +428,11 @@ static int cmh_hmac_final(struct ahash_request *req)
>  
>  	/*
>  	 * Raw key: pack SYS_CMD_WRITE(SYS_REF_TEMP) into the
>  	 * same VCQ so the key write + HMAC are atomic.
>  	 */
>  	key_dma = tctx->key.raw.dma;
>  	vcq_add_sys_write(&cmds[idx++], SYS_REF_TEMP, (u64)key_dma,
>  			  SYS_REF_NONE, tctx->key.raw.len,
>  			  tctx->key.raw.sys_type);

[Severity: High]
If the system has multiple HC instances, cmh_core_select_instance() will
return a dynamically selected core_id.

Will this SYS_CMD_WRITE command dispatch the key to the static core cached
earlier in cmh_hmac_setkey(), while the HC_CMD_HMAC command executes on the
new dynamically selected core, causing cryptographic failures?

>  	key_ref = SYS_REF_TEMP;
>  	keylen = tctx->key.raw.len;
>  	d = cmh_core_select_instance(CMH_CORE_HC);
[ ... ]
> @@ -513,11 +513,9 @@ static int cmh_hmac_export(struct ahash_request *req, void *out)
>  {
>  	struct cmh_hmac_reqctx *rctx = ahash_request_ctx(req);
>  	struct cmh_hmac_export_state *state = out;
>  	struct cmh_hmac_chunk *chunk;
>  	u32 offset = 0;
>  
>  	if (rctx->total_len > CMH_HMAC_EXPORT_MAX)
>  		return -ENOSPC;

[Severity: High]
Will this fail the export operation for any state exceeding ~4KB?

Generic consumers like af_alg expect state serialization to always succeed if
given a buffer matching the declared statesize.

>  
>  	state->total_len = rctx->total_len;
[ ... ]
> @@ -529,19 +529,14 @@ static int cmh_hmac_import(struct ahash_request *req, const void *in)
>  {
>  	struct crypto_ahash *tfm = crypto_ahash_reqtfm(req);
>  	struct cmh_hmac_tfm_ctx *tctx = crypto_ahash_ctx(tfm);
>  	struct cmh_hmac_reqctx *rctx = ahash_request_ctx(req);
>  	const struct cmh_hmac_export_state *state = in;
>  	struct cmh_hmac_chunk *chunk;
>  
>  	/*
>  	 * Do NOT call free_chunks() here: the crypto API does not
>  	 * guarantee the request context is in a valid state before
>  	 * import(), so the list pointers may be stale or invalid.
>  	 * Re-initialize from scratch instead.  Any pre-existing chunks
>  	 * are tracked on tctx->all_chunks and freed in cra_exit.
>  	 */
>  	rctx->info = cmh_hmac_get_info(tfm);
>  	rctx->error = 0;
>  	INIT_LIST_HEAD(&rctx->chunks);

[Severity: High]
Similarly to cmh_hmac_init(), will this overwrite the chunk list and leak
previously tracked chunks if an active request is reused?

>  	rctx->num_chunks = 0;
>  	rctx->total_len = 0;
>  
>  	if (state->total_len > CMH_HMAC_EXPORT_MAX)
>  		return -EINVAL;
>  
>  	if (state->total_len) {
>  		chunk = kmalloc(sizeof(*chunk) + state->total_len, GFP_KERNEL);

[Severity: High]
Can this trigger a sleep-in-atomic BUG?

The ahash import() operation can be invoked from atomic or interrupt contexts
(such as the IPsec network stack processing packets in softirq), making an
unconditional GFP_KERNEL allocation unsafe.

>  		if (!chunk)
>  			return -ENOMEM;
[ ... ]
> @@ -578,13 +578,11 @@ static void cmh_hmac_cra_exit(struct crypto_tfm *tfm)
>  {
>  	struct cmh_hmac_tfm_ctx *tctx = crypto_tfm_ctx(tfm);
>  	struct cmh_hmac_chunk *chunk, *tmp;
>  
>  	/* Free any orphaned chunks (e.g. testmgr export/reimport poison) */
>  	spin_lock_bh(&tctx->chunk_lock);
>  	list_for_each_entry_safe(chunk, tmp, &tctx->all_chunks, tfm_node) {
>  		list_del(&chunk->tfm_node);

[Severity: High]
Since orphaned chunks are only freed here when the entire TFM is destroyed,
couldn't a user trivially trigger unbounded memory exhaustion (OOM) by
repeatedly accumulating data via AF_ALG sockets and aborting requests?

>  		kfree_sensitive(chunk);
>  	}

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