Re: [PATCH v3 02/19] crypto: cmh - add core platform driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 9 potential issue(s) to consider:
- [Critical] Unconditional algorithm registration leads to a division by zero crash if the requested hardware core is not present.
- [High] Arming the transaction timeout timer after ringing the hardware doorbell creates a race condition leading to an orphaned timer and kernel panic on module unload.
- [High] Synchronous VCQ submission returns `-ETIMEDOUT` without transferring DMA buffer ownership to an orphan callback, leading to DMA memory corruption.
- [High] A TOCTOU race in `cmh_tm_submit_sync_tmo()` reading `msg->actual_mbx` allows bypassing the hardware ABORT command, leading to DMA use-after-free.
- [High] Driver teardown and suspend routines abandon in-flight transactions without halting the hardware, causing DMA writes into freed memory.
- [High] Synchronous submission timeout logic issues a blind ABORT which may incorrectly stop an unrelated transaction, leaving the timed-out transaction in the hardware queue.
- [High] Watchdog-initiated FLUSH recovery races with the Transaction Manager, allowing new VCQs to be submitted to an actively flushing mailbox.
- [Medium] The response handler fails to probe if mailboxes share an interrupt line, breaking the shared IRQ path.
- [Medium] Missing bounds check on embedded VCQ headers causes an out-of-bounds read of kernel heap memory during multi-VCQ dispatch.
--

--- Patch [2]: [PATCH v3 02/19] crypto: cmh - add core platform driver ---
commit c465b95a1402d4dad19cb4fdc8839741bcef0196
Author: Alex Ousherovitch <aousherovitch@rambus.com>
Subject: crypto: cmh - add core platform driver

This commit adds the core platform driver for the Rambus CryptoManager Hub
hardware accelerator. It provides mailbox interface management, transaction
handling, threaded IRQ completions, and device tree parsing.

> diff --git a/drivers/crypto/cmh/cmh_main.c b/drivers/crypto/cmh/cmh_main.c
> --- a/drivers/crypto/cmh/cmh_main.c
> +++ b/drivers/crypto/cmh/cmh_main.c
> @@ -215,8 +215,8 @@ static int cmh_probe(struct platform_device *pdev)
>  	/* Register hash algorithms with the kernel crypto API */
>  	ret = cmh_hash_register();
>  	if (ret)
>  		goto err_hash_register;

[Severity: Critical]
Can this unconditional algorithm registration lead to a division by zero
crash? If the underlying hardware core is physically absent, its
num_instances remains 0. If userspace requests an operation via AF_ALG, it
will eventually call cmh_core_select_instance() which performs a modulo
by num_instances.

> diff --git a/drivers/crypto/cmh/cmh_rh.c b/drivers/crypto/cmh/cmh_rh.c
> --- a/drivers/crypto/cmh/cmh_rh.c
> +++ b/drivers/crypto/cmh/cmh_rh.c
> @@ -462,11 +462,11 @@ static void cmh_rh_watchdog_fn(struct timer_list *t)
>  [ ... ]
>  					cmh_reg_write32(MBX_COMMAND_FLUSH,
>  							base,
>  							R_MBX_COMMAND);
>  					cmh_rh_poke_tail(base);
>  					cmh_rh_drain_mbx(i, -EIO);

[Severity: High]
Does this watchdog-initiated flush sequence race with the transaction
manager? Because this runs in a timer context, it immediately drains the
mailbox without waiting for the hardware flush to complete. 

This wakes the TM thread, which sees empty queue pointers and selects the
actively flushing mailbox as available, allowing new VCQs to be submitted
while the hardware is still flushing. Could this corrupt the mailbox ring
state? (The same sequence is also present in cmh_rh_force_drain_mbx).

> @@ -840,11 +840,11 @@ static int cmh_rh_resolve_irqs(struct cmh_config *cfg)
>  {
>  [ ... ]
>  	for (i = 0; i < cfg->mbx_count; i++) {
>  		rh.irqs[i] = cfg->mailboxes[i].irq;
>  		dev_dbg(cmh_dev(), "rh: MBX%u -> IRQ %d\n", i, rh.irqs[i]);
>  	}
>  
>  	rh.nirqs = cfg->mbx_count;
>  	return 0;
>  }
>  
>  int cmh_rh_init(struct cmh_config *cfg)
>  {
>  [ ... ]
>  		unsigned long irqflags = (rh.nirqs == 1 && cfg->mbx_count > 1)
>  					  ? IRQF_SHARED : 0;

[Severity: Medium]
Will this logic ever allow the shared IRQ path to be active? Because
cmh_rh_resolve_irqs() unconditionally sets rh.nirqs to cfg->mbx_count,
the condition for IRQF_SHARED evaluates to false. 

This prevents probe on platforms where mailboxes share an interrupt line,
as the second request_threaded_irq() call will fail with -EBUSY.

> diff --git a/drivers/crypto/cmh/cmh_txn.c b/drivers/crypto/cmh/cmh_txn.c
> --- a/drivers/crypto/cmh/cmh_txn.c
> +++ b/drivers/crypto/cmh/cmh_txn.c
> @@ -263,11 +263,11 @@ void cmh_txn_finish(struct transaction_obj *txn, int error)
>  {
>  	int old;
>  
>  	old = atomic_cmpxchg(&txn->state, TXN_INFLIGHT, TXN_COMPLETE);
>  
>  	/* Dequeue the timer if still pending; drop timer ref if we did */
>  	if (timer_delete(&txn->timeout_timer))
>  		txn_put(txn);

[Severity: High]
Could this timer cleanup logic race with the timer setup in submit_vcq()?

If the hardware completes the VCQ instantly after the doorbell is rung
but before mod_timer() is called, cmh_txn_finish() will be called while
the timer is not yet pending. As a result, timer_delete() returns 0 and
leaves the timer reference. 

The TM thread then resumes and arms the timer on the already popped
transaction, creating an orphaned timer that can trigger a kernel panic
on module unload.

> @@ -472,11 +472,11 @@ struct core_dispatch cmh_core_select_instance(enum cmh_core_type type)
>  {
>  	struct core_type_info *ct;
>  [ ... ]
>  	ct = &core_types[type];
>  	idx = (u32)atomic_inc_return(&ct->next_instance) % ct->num_instances;

[Severity: Critical]
Does this modulo operation cause a division by zero crash if the
hardware core is physically absent? Since algorithm registrations in
cmh_probe() happen unconditionally, userspace can request an operation
for an absent core, leaving num_instances at 0 here.

> @@ -682,11 +682,11 @@ static int submit_vcq(struct command_msg *msg, u32 mbx_idx)
>  {
>  [ ... ]
>  		if (num_vcqs == 1) {
>  			vcq_cmds = msg->vcq_count;
>  		} else {
>  			const struct vcq_hdr *hdr =
>  				(const struct vcq_hdr *)&cmds[offset].hwc;
>  			vcq_cmds = hdr->cmds;
>  		}
>  
>  		copy_size = vcq_cmds * sizeof(struct vcq_cmd);
>  		if (copy_size > stride_bytes) {

[Severity: Medium]
Is a bounds check needed here to prevent an out-of-bounds read? For a
multi-VCQ message, hdr->cmds is read from the array to determine the
copy_size. 

Without verifying that the accumulated offset + vcq_cmds does not exceed
msg->vcq_count, a malformed array could cause the driver to read past the
end of the msg->vcq_data allocation.

> @@ -782,11 +782,11 @@ static int submit_vcq(struct command_msg *msg, u32 mbx_idx)
>  	/* Ring doorbell: advance tail by number of VCQs submitted */
>  	cmh_reg_write32(tail + num_vcqs, mbx->reg_base, R_MBX_QUEUE_TAIL);
>  
>  	/* Arm per-request timeout after doorbell (async only) */
>  	if (msg->timeout_jiffies)
>  		mod_timer(&txn->timeout_timer,
>  			  jiffies + msg->timeout_jiffies);

[Severity: High]
This appears to be the other side of the timer race condition noted
earlier. Could arming the timer after ringing the doorbell create a
window where the hardware completes the command before the timer is
armed, leading to the orphaned timer issue?

> @@ -1067,11 +1067,11 @@ void cmh_tm_cleanup(void)
>  {
>  [ ... ]
>  				old = atomic_cmpxchg(&txn->state,
>  						     TXN_INFLIGHT,
>  						     TXN_COMPLETE);
>  				if (txn->complete) {
>  					if (old == TXN_INFLIGHT)
>  						txn->complete(txn->completion_data,
>  							      -ECANCELED);

[Severity: High]
Does abandoning in-flight transactions here cause DMA writes into freed
memory? The transactions are force-completed with -ECANCELED and their
callers will proceed to free the DMA buffers. 

However, no ABORT or FLUSH command is issued to halt the hardware
execution, meaning the engine will eventually DMA its output to the
freed memory. The same pattern exists in cmh_tm_quiesce().

> @@ -1433,11 +1433,11 @@ int cmh_tm_submit_sync_tmo(struct vcq_cmd *vcq_cmds, u32 vcq_count,
>  			   u32 num_vcqs, s32 target_mbx,
>  			   unsigned long timeout_hz)
>  {
>  [ ... ]
>  		if (cmh_tm_try_cancel_command(msg)) {
>  [ ... ]
>  		} else {
>  			/*
>  			 * TM has dequeued msg and the VCQ is in-flight.
>  			 * Issue MBX_COMMAND_ABORT to force-stop the VCQ;
>  [ ... ]
>  			s32 abrt_mbx = READ_ONCE(msg->actual_mbx);
>  
>  			if (abrt_mbx >= 0 &&
>  			    (u32)abrt_mbx < tm.cfg->mbx_count) {
>  				dev_warn(cmh_dev(),
>  					 "tm: aborting mbx[%d] cmd=0x%08x\n",
>  					 abrt_mbx, msg->command_id);
>  				cmh_rh_abort_mbx((u32)abrt_mbx);
>  			}

[Severity: High]
Is there a Time-of-Check to Time-of-Use race here that bypasses the
hardware abort? If the TM thread has dequeued the message but is
delayed before writing msg->actual_mbx, READ_ONCE() returns -1. The
waiter skips sending the ABORT and returns -ETIMEDOUT, freeing the DMA
buffers while the TM thread resumes and submits the command to the
hardware.

Additionally, could this blind abort terminate the wrong transaction?
The ABORT command stops the operation at the head of the mailbox queue.
If this timed-out transaction is queued behind a slow operation, this
aborts the slow operation instead, leaving this transaction in the
hardware queue to eventually corrupt freed memory.

>  [ ... ]
>  			left = wait_for_completion_timeout(&sync->done,
>  							   5 * HZ);
>  			if (!left) {
>  				/*
>  				 * ABORT did not complete within 5 s -- HW
>  				 * is wedged.  The eSW may still be writing
>  				 * to DMA buffers owned by the caller, so we
>  				 * cannot let the caller free them.  Transfer
>  				 * ownership to the sync_ctx orphan mechanism;
>  [ ... ]
>  				dev_err(cmh_dev(),
>  					"tm: abort timeout (5s) cmd=0x%08x - DMA buffers orphaned\n",
>  					msg->command_id);
>  			}
>  			cmh_sync_ctx_put(sync);  /* drop waiter ref */
>  			command_msg_put(msg);    /* drop waiter ref on msg */
>  		}
>  		return -ETIMEDOUT;
>  	}

[Severity: High]
Does returning -ETIMEDOUT here without transferring DMA buffer ownership
lead to memory corruption? When this expires, the caller unmaps and
frees the DMA buffers. 

However, the function does not attach an orphan_cb to the sync context
here. If the hardware is merely slow rather than fully wedged, it will
eventually complete and DMA its output into the now-freed memory.

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