Re: [PATCH v4 11/19] crypto: cmh - add DRBG hwrng

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Missing hardware presence check in `cmh_rng_register` causes indefinite spinning if DRBG is absent.
- [High] Module unload frees in-flight DRBG DMA buffers without stopping the hardware, causing a DMA use-after-free.
- [Medium] Small `kmalloc` buffer mapped for DMA risks cacheline sharing and memory corruption.
--

commit 888e90a741e40c8e3f25d31a12ecb6c6108685b4
Author: Alex Ousherovitch <aousherovitch@rambus.com>

crypto: cmh - add DRBG hwrng
    
Register the CMH DRBG core (core ID 0x0f) as an hwrng provider.
The hardware implements a NIST SP 800-90A compliant DRBG with
automatic self-seeding.

> diff --git a/drivers/crypto/cmh/cmh_rng.c b/drivers/crypto/cmh/cmh_rng.c
> index 0000000000000..a73634024e135
> --- /dev/null
> +++ b/drivers/crypto/cmh/cmh_rng.c

[ ... ]

> +static int cmh_rng_read(struct hwrng *rng, void *data, size_t max, bool wait)
> +{
> +	struct cmh_dma_orphan *orphan;
> +	struct vcq_cmd vcq[DRBG_READ_VCQ_CMDS];
> +	dma_addr_t dma_addr;
> +	void *dmabuf;
> +	size_t nbytes;
> +	int ret;

[ ... ]

> +	nbytes = min_t(size_t, max, CMH_DRBG_MAX_GENERATE);
> +
> +	orphan = kmalloc_obj(*orphan, GFP_KERNEL);
> +	if (!orphan)
> +		return -ENOMEM;
> +
> +	dmabuf = kmalloc(nbytes, GFP_KERNEL);
> +	if (!dmabuf) {
> +		kfree(orphan);
> +		return -ENOMEM;
> +	}
> +
> +	dma_addr = cmh_dma_map_single(dmabuf, nbytes, DMA_FROM_DEVICE);

[Severity: Medium]
Since dmabuf is a small allocation (up to 32 bytes) from the generic kmalloc
cache, does mapping it for DMA risk cacheline sharing and memory corruption?

On architectures without SWIOTLB bounce-buffering for unaligned kmallocs,
this violates DMA API requirements for cacheline alignment and risks adjacent
object corruption upon unmap or invalidate.

[ ... ]

> +	/*
> +	 * Use the noabort variant: if the MBX is occupied by a slow
> +	 * operation (e.g. SLH-DSA sign at 120 s), we must not issue
> +	 * MBX_COMMAND_ABORT -- that would kill the unrelated in-flight
> +	 * VCQ.  On timeout with an in-flight VCQ (-EINPROGRESS), the
> +	 * orphan callback defers DMA cleanup until the RH fires.
> +	 */
> +	ret = cmh_tm_submit_sync_noabort(vcq, DRBG_READ_VCQ_CMDS, 1,
> +					 msecs_to_jiffies(drbg_timeout_ms),
> +					 cmh_dma_orphan_free, orphan);
> +	if (ret == -EINPROGRESS) {
> +		/*
> +		 * The orphan callback owns dmabuf and frees it on VCQ
> +		 * completion.  Return 0 (not -EAGAIN): .read() only runs with
> +		 * wait=true (see the !wait early return above), and the hwrng
> +		 * core forwards a negative errno straight to a blocking read
> +		 * whereas a 0 return makes it retry.
> +		 */
> +		return 0;
> +	}

[Severity: High]
Does this code introduce a DMA use-after-free during module unload?

If cmh_rng_read() returns 0 on a timeout (-EINPROGRESS), the hardware continues
executing and an orphan callback (cmh_dma_orphan_free) is set. During module
unload, cmh_tm_cleanup() forcefully completes timed-out transactions to drain
the queues, triggering the orphan callback which unmaps and frees the DMA
buffer:

drivers/crypto/cmh/cmh_dma.c:cmh_dma_orphan_free() {
    cmh_dma_unmap_single(o->addr, o->len, o->dir);
    kfree_sensitive(o->buf);
}

Because the hardware lacks a stop primitive and is still running, when it
finally completes, will it write random bytes into the freed physical memory?

[ ... ]

> +int cmh_rng_register(struct platform_device *pdev)
> +{
> +	struct vcq_cmd cfg_vcq[DRBG_CONFIG_VCQ_CMDS];
> +	int ret;
> +
> +	/*
> +	 * The hwrng core elevates a zero quality to full trust (1024) for
> +	 * a hardware RNG, so cmh_hwrng.quality is left at 0.  Set it in the
> +	 * hwrng initializer to lower the entropy estimate if a platform
> +	 * requires it.
> +	 *
> +	 * DRBG CONFIG is a management-host operation.  Attempt it: it
> +	 * succeeds in stateless mode (any host) or when we are the
> +	 * management host.  On -EPERM (not the management host) continue
> +	 * without error -- GENERATE works once the management host has
> +	 * configured the DRBG.
> +	 */
> +	vcq_set_header(&cfg_vcq[0], DRBG_CONFIG_VCQ_CMDS);
> +	vcq_add_drbg_reset(&cfg_vcq[1]);
> +	vcq_add_drbg_config(&cfg_vcq[2], CMH_DRBG_ENTROPY_RATIO,
> +			    CMH_DRBG_SECURITY_STRENGTH);

[Severity: High]
Is it intentional to interact with the DRBG hardware here without checking if
the DRBG core is actually present?

Other CMH registration functions seem to use cmh_core_present(CMH_CORE_DRBG)
to guard against absent hardware. If the DRBG core is not present, commands
could time out. The function ignores the -ETIMEDOUT error, warns, and registers
the device anyway.

If a local process then reads from /dev/hwrng, cmh_rng_read() will time out
and return 0, which could force the hwrng core into an infinite retry loop,
causing readers to spin indefinitely.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260825221539.255951-1-aousherovitch@rambus.com?part=11