Re: [PATCH v5 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] rng: DMA use-after-free on module unload due to un-aborted DRBG operation
- [Medium] rng: infinite retry loop on fatal hardware error in cmh_rng_read()
- [Medium] rng: mailbox queue exhaustion from orphaned DRBG requests
--

commit 8d9076a5e8784ced0b9563b435b225e28e6737e2
Author: Alex Ousherovitch <aousherovitch@rambus.com>

crypto: cmh - add DRBG hwrng

This commit registers the CMH DRBG core as a hardware RNG provider. It adds
synchronous reads that map DMA buffers and submit VCQ commands through the
transaction manager to gather entropy from the device.

> diff --git a/drivers/crypto/cmh/cmh_rng.c b/drivers/crypto/cmh/cmh_rng.c
> new file mode 100644
> index 0000000000000..a73634024e135
> --- /dev/null
> +++ b/drivers/crypto/cmh/cmh_rng.c
[ ... ]
> +	/*
> +	 * 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);

[Severity: High]
Could this cause a DMA use-after-free during module unload? Since
cmh_tm_submit_sync_noabort() leaves the hardware operation running in the
background, if the cmh module is unloaded, cmh_tm_cleanup() will force-cancel
the transaction and trigger cmh_dma_orphan_free(). This unmaps and frees the
DMA buffer while the hardware accelerator is still actively writing to it.

> +	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: Medium]
Does returning 0 here cause mailbox queue exhaustion? When 0 is returned
on a timeout, the hwrng core immediately retries the read. Since the
hardware might be blocked by a slow operation, won't this rapidly submit
more DRBG requests until the Command Message Queue is completely filled
with orphaned requests, blocking all other subsystems?

[ ... ]
> +		switch (ret) {
> +		case -EAGAIN:
> +		case -EBUSY:
> +		case -ETIMEDOUT:
> +		case -EIO:
> +		/*
> +		 * -ENODEV: the TM is not running -- occurs when the
> +		 * hwrng kthread (PF_NOFREEZE, not frozen during
> +		 * suspend) calls .read() while the device is suspended.
> +		 * Treat as transient: the TM restarts on resume.
> +		 */
> +		case -ENODEV:
> +			dev_dbg_ratelimited(cmh_dev(),
> +					    "rng: transient DRBG failure (rc=%d)\n",
> +					    ret);
> +			kfree_sensitive(dmabuf);
> +			/*
> +			 * Back off before the hwrng core re-issues the
> +			 * blocking read: rng_dev_read() loops on a 0 return
> +			 * with only a conditional need_resched, so a persistent
> +			 * transient fault would otherwise spin the CPU.
> +			 */
> +			msleep(CMH_DRBG_RETRY_BACKOFF_MS);
> +			return 0;

[Severity: Medium]
Will this create an infinite retry loop for fatal hardware errors? By
masking -EIO to 0 and treating it as a transient error, rng_dev_read()
will retry the read indefinitely if the accelerator encounters a persistent
failure. Should fatal errors be propagated to break the loop instead of
returning 0?

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