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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 11 potential issue(s) to consider:
- [High] Force-completing transactions without a synchronous hardware stop allows the hardware to write into freed DMA buffers.
- [High] Unsynchronized hardware FLUSH commands race with TM thread VCQ submissions, corrupting the DMA queue.
- [High] Concurrent modification of the CMQ drain list during timeout cancellation corrupts list pointers.
- [High] TM thread temporarily detaches messages from the CMQ, causing synchronous timeouts to bypass ABORT and allow DMA into freed memory.
- [High] `cmh_tm_quiesce()` frees transaction objects while the active Response Handler accesses them.
- [High] The per-transaction timeout timer is armed after the hardware doorbell, creating a race that leaves the timer armed after transaction completion.
- [High] Invoking completion callbacks while holding the `tm.cmq_lock` spinlock creates a recursive deadlock hazard.
- [High] TOCTOU race clearing the `pending` flag in `cmh_rh_thread` orphans hardware interrupt bits.
- [High] Hardware DMA structures lack endianness conversions, breaking the driver on Big-Endian hosts.
- [Medium] Sysfs attributes 'product' and 'algorithms' claimed in the commit message are absent from the code.
- [Medium] The driver masks `-EPROBE_DEFER` from `of_irq_get()`, permanently degrading to polling mode.
--

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

crypto: cmh - add core platform driver

This commit introduces the core platform driver for the CRI CryptoManager Hub
hardware crypto accelerator. It sets up mailbox queues, a transaction manager,
and response handlers for asynchronous crypto operations. The driver uses DMA
and MMIO to dispatch cryptographic commands to the hardware cores.

> - Sysfs attributes (fw_version, hw_version, product, algorithms)

[Severity: Medium]
The commit message mentions 'product' and 'algorithms' sysfs attributes, but
are these actually implemented in the code? Looking at the cmh_sysfs_attrs
array in cmh_sysfs.c, only fw_version, hw_version, boot_status, mbx_available,
and mbx_count appear to be present.

> diff --git a/drivers/crypto/cmh/include/cmh_vcq.h b/drivers/crypto/cmh/include/cmh_vcq.h
> --- a/drivers/crypto/cmh/include/cmh_vcq.h
> +++ b/drivers/crypto/cmh/include/cmh_vcq.h
[ ... ]
> struct dma_scattergather_item {
> 	u64 lli;
> 	u64 src;
> 	u64 dst;
> 	u64 len;
> };

[Severity: High]
Will this driver operate correctly on big-endian architectures? The DMA
scatter-gather items and VCQ command structures are defined using native u64
types. If the hardware expects little-endian layout, do we need explicit
endianness conversions here and when copying data to the DMA slots in
cmh_dma_build_sg() and submit_vcq()?

> 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
[ ... ]
> 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]
Can a concurrent mailbox flush operation corrupt the hardware queue here? If a
watchdog or debugfs routine issues a flush command asynchronously after the TM
thread reads the tail but before writing the new tail to the doorbell, could
the hardware reset the tail to zero only for it to be overwritten with an
advanced value pointing to empty DMA slots?

[Severity: High]
Is there a race condition with the timeout timer being armed? The doorbell
is rung before mod_timer() is called. If the hardware completes the operation
extremely quickly, could cmh_txn_finish() process the completion and delete
the timer before it gets armed here, leaving an active timer on a freed
transaction?

[ ... ]
> static int cmh_tm_thread(void *data)
> {
[ ... ]
> 		/* Dequeue one command message */
> 		spin_lock_irqsave(&tm.cmq_lock, flags);
[ ... ]
> 		msg = list_first_entry(&tm.cmq, struct command_msg, list);
> 		list_del_init(&msg->list);
> 		cmq_depth--;

[Severity: High]
Could this temporary removal from the message queue lead to a use-after-free
on timeouts? The message is detached from the list before checking mailbox
availability. If a synchronous caller times out during this detached window,
the cancellation attempt will fail. Because the actual mailbox is still
unassigned, the timeout handler skips sending an abort command and frees the
DMA buffers. However, the thread might later re-add and submit this message,
instructing hardware to write to freed memory.

[ ... ]
> 		if (!list_empty(&tm.backlog)) {
> 			struct command_msg *bl;
[ ... ]
> 			if (bl->complete)
> 				bl->complete(bl->completion_data,
> 					     -EINPROGRESS);
> 		}
> 
> 		spin_unlock_irqrestore(&tm.cmq_lock, flags);

[Severity: High]
Can this recursive spinlock acquisition cause a deadlock? The code calls the
completion callback with the in-progress status while holding the command
queue lock. If the crypto framework consumer responds to this callback by
submitting a new request, won't the submission attempt to acquire the exact
same spinlock on the same CPU?

[ ... ]
> static void cmh_tm_stop_and_drain_cmq(void)
> {
[ ... ]
> 	list_for_each_entry_safe(msg, tmp_msg, &cmq_drain, list) {
> 		list_del(&msg->list);
> 		if (msg->complete)
> 			msg->complete(msg->completion_data, -ECANCELED);
> 		command_msg_put(msg);
> 	}

[Severity: High]
Is it safe to iterate over the spliced local drain list without holding a
lock here? If a synchronous request times out simultaneously, the cancellation
function will acquire the queue lock, see that the message's list node isn't
empty (since it is on the local drain list), and call list_del_init(). This
concurrent modification could corrupt the list pointers during iteration.

[ ... ]
> void cmh_tm_quiesce(void)
> {
[ ... ]
> 			list_for_each_entry_safe(txn, tmp_txn, &drain, list) {
> 				list_del(&txn->list);
> 
> 				if (timer_delete_sync(&txn->timeout_timer))
> 					txn_put(txn);
> 
> 				old = atomic_cmpxchg(&txn->state,
> 						     TXN_INFLIGHT,
> 						     TXN_COMPLETE);
> 				if (txn->complete) {
> 					if (old == TXN_INFLIGHT)
> 						txn->complete(txn->completion_data,
> 							      -ECANCELED);
[ ... ]
> 				txn_put(txn);
> 			}

[Severity: High]
Could this lead to a use-after-free race with the response handler?
Transactions are being iterated and freed via txn_put() without holding the
process lock. Concurrently, the active watchdog might peek at the head
transaction. If the transaction is freed here before the watchdog reads its
state, memory corruption could result.

[Severity: High]
Does completing these transactions with the cancelled status leave the
hardware in a state where it might overwrite freed memory? The driver
forcefully completes abandoned requests here (and similarly during module
cleanup), which prompts callers to free their DMA buffers. Since the hardware
engines haven't been synchronously stopped, could they continue executing and
write into these newly freed buffers?

> 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
[ ... ]
> void cmh_rh_force_drain_mbx(u32 mbx_idx)
> {
[ ... ]
> 	cmh_reg_write32(MBX_COMMAND_FLUSH, base, R_MBX_COMMAND);
> 	cmh_rh_poke_tail(base);
> 	cmh_rh_drain_mbx(mbx_idx, -ECANCELED);

[Severity: High]
Similar to the quiesce behavior, does issuing a flush and immediately
draining the mailbox allow the hardware to access freed memory? If the
flush command completes asynchronously, the hardware might still be accessing
the buffers when the drain completes the transactions, prompting callers
to release them prematurely.

[ ... ]
> static irqreturn_t cmh_rh_thread(int irq, void *data)
> {
[ ... ]
> 			irq_bits = (u32)atomic_xchg(&rh.mbx[i].irq_bits, 0);
> 			WRITE_ONCE(rh.mbx[i].pending, false);

[Severity: High]
Is there a time-of-check to time-of-use race window here? The thread reads
the interrupt bits using atomic_xchg and subsequently clears the pending flag.
If a hardware interrupt occurs strictly between these two lines, the hardirq
handler will set the pending flag to true, but this code will immediately
overwrite it to false. Could this result in orphaned interrupt bits and missed
completions?

[ ... ]
> static int cmh_rh_resolve_irqs(struct cmh_config *cfg)
> {
[ ... ]
> 		int virq = of_irq_get(np, dt_idx);
> 
> 		if (virq <= 0) {
> 			dev_warn(cmh_dev(), "rh: failed to map IRQ for MBX%u (DT index %d, rc=%d)\n",
> 				 i, dt_idx, virq);
> 			return -ENODEV;
> 		}

[Severity: Medium]
Does swallowing the deferral error cause issues with interrupt controller
readiness? If the IRQ controller is not yet ready, of_irq_get() will return
-EPROBE_DEFER, but this code converts it to -ENODEV and falls back to polling
permanently. Should this propagate the deferral so the driver can be probed
again later?

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