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

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Unbounded kernel memory leak due to orphaned dynamically allocated chunks when a hash request is aborted before `.final()` or re-initialized/imported.
- [Medium] Potential out-of-bounds write in `cmh_hmac_export` when exporting the software fallback state due to missing statesize validation.
- [Medium] Mapping an unaligned interior pointer (`chunk->data`) for DMA violates DMA API requirements and risks cacheline corruption or runtime warnings.
--

--- Patch [5]: [PATCH v3 05/19] crypto: cmh - add HMAC ahash ---
commit 29cce2e66e05b4cda73d07e16e2b43d65f198bd1
Author: Alex Ousherovitch <[email protected]>

crypto: cmh - add HMAC ahash

This commit registers HMAC-SHA-2 and HMAC-SHA-3 ahash algorithms using the
CMH hash core. It implements a self-contained transaction model with
software fallback for large accumulated inputs and state cloning.

> diff --git a/drivers/crypto/cmh/cmh_hmac.c b/drivers/crypto/cmh/cmh_hmac.c
> new file mode 100644
> index 0000000000000..b43b358c35963
> --- /dev/null
> +++ b/drivers/crypto/cmh/cmh_hmac.c
> @@ -0,0 +1,865 @@
[ ... ]
> +struct cmh_hmac_chunk {
> +	struct list_head  list;
> +	struct list_head  tfm_node; /* per-tfm orphan tracking */
> +	u32               len;
> +	u8                data[];
> +};

[Severity: Medium]
Does mapping this unaligned data array violate DMA API requirements?

The data array sits at an unaligned offset (36 bytes), which shares a
cacheline with the struct's metadata.

Later, in cmh_hmac_build_sg(), this unaligned interior pointer is passed to
the DMA mapping subsystem:

    list_for_each_entry(chunk, &rctx->chunks, list) {
        bufs[i].data = chunk->data;
        bufs[i].len = chunk->len;
        i++;
    }
    sgm = cmh_dma_build_sg(bufs, rctx->num_chunks, gfp);

Could this risk cacheline corruption on non-coherent architectures and
trigger CONFIG_DMA_API_DEBUG warnings?

[ ... ]
> +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 cmh_hmac_init() is called on an active request, does this unconditionally
overwrite the list head and permanently lose track of any locally buffered
chunks?

A similar pattern appears in cmh_hmac_import().

[ ... ]
> +static int cmh_hmac_update(struct ahash_request *req)
> +{
[ ... ]
> +	chunk = kmalloc(sizeof(*chunk) + req->nbytes,
> +			req->base.flags & CRYPTO_TFM_REQ_MAY_SLEEP ?
> +			GFP_KERNEL : GFP_ATOMIC);
> +	if (!chunk) {
> +		rctx->error = -ENOMEM;
> +		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);
> +	spin_unlock_bh(&tctx->chunk_lock);

[Severity: High]
What happens to these allocations if an unprivileged user creates an
AF_ALG socket, sends data to trigger this allocation, and then closes the
socket before calling recv() to trigger final()?

Since the crypto API has no per-request destructor, and these chunks are
added to the global tctx->all_chunks list, it appears they are only freed
in cmh_hmac_cra_exit():

    list_for_each_entry_safe(chunk, tmp, &tctx->all_chunks, tfm_node) {
        list_del(&chunk->tfm_node);
        kfree_sensitive(chunk);
    }

Could a user repeat this in a loop on new accepted sockets to indefinitely
consume kernel memory until the system runs out of memory?

[ ... ]
> +static int cmh_hmac_export(struct ahash_request *req, void *out)
> +{
[ ... ]
> +	if (rctx->switched) {
> +		state->format = CMH_HMAC_FMT_FB;
> +		return crypto_ahash_export(cmh_hmac_fb_req(rctx), state->data);
> +	}

[Severity: Medium]
Is there a risk of an out-of-bounds write here if the generic software
fallback algorithm allocated in cmh_hmac_cra_init() has a state size larger
than CMH_HMAC_EXPORT_MAX?

The state buffer size is computed using crypto_ahash_statesize(tfm), but
there appears to be no structural validation in cmh_hmac_cra_init() ensuring
the allocated fallback's state actually fits within this fixed-size limit
before it is exported.

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