Re: [PATCH 2/5] crypto: cmh - add core platform driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [Critical] cmh: Race between asynchronous MBX_COMMAND_FLUSH and new TM submissions
- [Critical] cmh: TOCTOU race in sync timeout skips MBX_COMMAND_ABORT causing DMA corruption
- [High] cmh: Missing timeout_jiffies init for sync submissions permanently wedges mailbox
- [High] dma: Memory corruption risk when orphan tracking allocation fails
- [Medium] cmh: Missing R_MBX_HOST_INFO write during MBX initialization
--

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

crypto: cmh - add core platform driver

Add the Rambus CryptoManager Hub (CMH) hardware crypto accelerator
core platform driver. The driver manages hardware configuration, VCQ
command submission, and asynchronous completions.

> diff --git a/drivers/crypto/cmh/cmh_dma.c b/drivers/crypto/cmh/cmh_dma.c
> --- a/drivers/crypto/cmh/cmh_dma.c
> +++ b/drivers/crypto/cmh/cmh_dma.c
[ ... ]
> @@ -424,4 +424,4 @@ void cmh_dma_free_out(void *buf, dma_addr_t dma, size_t len,
>  		/* Rare: cannot park without a descriptor -- best-effort free. */
>  		dev_warn_ratelimited(cmh_dev(),
>  				     "dma: park alloc failed, freeing output buffer\n");
>  	}
>  
>  	cmh_dma_unmap_single(dma, len, dir);
>  	kfree_sensitive(buf);

[Severity: High]
Does this fallback path risk a DMA memory corruption regression if the
allocation fails?

Because the hardware lacks a DMA-stop primitive, it may still be actively
writing to this physical address. Freeing the memory back to the slab
allocator could allow it to be reassigned to other kernel structures while
the hardware is still writing to it. Is it safer to intentionally leak the
memory here to prevent corruption?

> diff --git a/drivers/crypto/cmh/cmh_mqi.c b/drivers/crypto/cmh/cmh_mqi.c
> --- a/drivers/crypto/cmh/cmh_mqi.c
> +++ b/drivers/crypto/cmh/cmh_mqi.c
[ ... ]
> @@ -201,4 +201,4 @@ static int cmh_mbx_setup_one(struct cmh_mbx_config *mbx)
>  	/*
>  	 * Step 6: Readback verify critical registers.
>  	 * HOST_INFO is deliberately deferred to after verification -- writing
>  	 * it tells the CMH eSW "MBX is ready" and the CMH eSW may inspect
>  	 * (and clear) the queue registers immediately.
>  	 */
>  	ret = cmh_verify_reg(base, R_MBX_QUEUE_LO, addr_lo,
>  			     "QUEUE_LO", mbx->instance);

[Severity: Medium]
The comment mentions writing to the HOST_INFO register to signal to the
embedded software that the mailbox is ready. However, it looks like the
driver never actually performs the write to this register after the
verification steps, and only writes MBX_COMMAND_RUN.

Will this leave the embedded software unaware that the mailbox is active,
causing a regression on hardware configurations that depend on this signal?

> 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
[ ... ]
> @@ -429,4 +429,4 @@ static void cmh_rh_process_mbx(u32 mbx_idx, u32 new_head, u32 irq_bits)
>  		if (txn && txn->first_vcq_id != txn->last_vcq_id) {
>  			dev_warn_ratelimited(cmh_dev(),
>  					     "rh: mbx[%u] multi-VCQ error vcq=%u..%u -- FLUSH+drain\n",
>  					     mbx_idx, txn->first_vcq_id,
>  					     txn->last_vcq_id);
>  			cmh_rh_stat_inc_errors(mbx_idx);
>  			cmh_reg_write32(MBX_IRQ_MASK, base, R_MBX_INTERRUPT);
>  			cmh_reg_write32(MBX_COMMAND_FLUSH, base, R_MBX_COMMAND);
>  			cmh_rh_poke_tail(base);
>  			cmh_rh_drain_mbx(mbx_idx, error);
>  			return;

[Severity: High]
Does this sequence introduce a regression involving a race condition with the
transaction manager?

The hardware flush command appears to be processed asynchronously by the
embedded software, but the software queue is drained immediately without
waiting for the flush to complete or marking the mailbox as offline.

Could the transaction manager thread concurrently acquire the dispatch lock,
write a new transaction into the ring, and advance the hardware tail doorbell?
If so, when the embedded software eventually processes the flush, it might
discard the new queue state. The response handler could then interpret the
advanced head pointer as a successful execution, completing discarded
transactions with no error.

This same pattern also occurs in cmh_rh_watchdog_fn() and cmh_tm_flush_mbx().

> 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
[ ... ]
> @@ -1500,4 +1500,4 @@ int cmh_tm_submit_sync_tmo(struct vcq_cmd *vcq_cmds, u32 vcq_count,
>  	msg->command_id = vcq_cmds[1].id;  /* first real command's ID */
>  	msg->vcq_count  = vcq_count;
>  	msg->num_vcqs   = num_vcqs;
>  	msg->target_mbx = target_mbx;
>  	msg->actual_mbx = -1;
>  	msg->first_vcq_id = U32_MAX;
>  	msg->complete   = cmh_sync_complete;
>  	msg->completion_data = sync;
>  	refcount_set(&msg->refs, 2);       /* waiter + TM subsystem */

[Severity: High]
Is the timeout_jiffies field intentionally left uninitialized for synchronous
submissions?

Since the command_msg is allocated with kzalloc_obj, msg->timeout_jiffies
remains zero. The transaction manager thread skips arming the timeout timer
when this field is zero.

Because the watchdog's abort-stall detector explicitly relies on a transaction
reaching the TXN_TIMED_OUT state, which is only set by the timer callback,
could a stalled synchronous request remain in the TXN_INFLIGHT state
indefinitely and permanently stall the mailbox if the hardware wedges?

[ ... ]
> @@ -1546,4 +1546,4 @@ int cmh_tm_submit_sync_tmo(struct vcq_cmd *vcq_cmds, u32 vcq_count,
>  			 * cmh_rh_abort_mbx() serialises the write under
>  			 * rh_process_lock, preventing clobber of a
>  			 * concurrent RESTART/FLUSH from the watchdog.
>  			 */
>  			s32 abrt_mbx = READ_ONCE(msg->actual_mbx);
>  
>  			/*
>  			 * Only ABORT if our VCQ is the one currently at the
>  			 * mailbox HEAD.  ABORT stops whatever executes at
>  			 * HEAD, so aborting while a different (older,
>  			 * possibly long-running) transaction is at HEAD would
>  			 * kill an unrelated request.  If ours is queued behind
>  			 * it, the HW has not started ours yet -- leave it to
>  			 * the completion wait / watchdog rather than abort
>  			 * someone else.
>  			 */
>  			if (abrt_mbx >= 0 &&
>  			    (u32)abrt_mbx < tm.cfg->mbx_count) {
>  				void __iomem *abase =
>  					tm.cfg->mailboxes[abrt_mbx].reg_base;
>  				u32 head = cmh_reg_read32(abase,
>  							  R_MBX_QUEUE_HEAD);
>  
>  				if (READ_ONCE(msg->first_vcq_id) == head) {
>  					dev_warn(cmh_dev(),
>  						 "tm: aborting mbx[%d] cmd=0x%08x\n",
>  						 abrt_mbx, msg->command_id);
>  					cmh_rh_abort_mbx((u32)abrt_mbx);
>  				}
>  			}
>  
>  			/*
>  			 * Wait for the RH completion (ABORT triggers
>  			 * MBX_ERROR_IRQ within microseconds).  Fixed
>  			 * 5 s ceiling -- not configurable because if
>  			 * ABORT doesn't complete in this window the
>  			 * HW is wedged and more waiting won't help.
>  			 */
>  			left = wait_for_completion_timeout(&sync->done,
>  							   5 * HZ);

[Severity: High]
Does reading msg->actual_mbx here create a race condition with the transaction
manager?

If the transaction manager thread has dequeued the message but is preempted
before setting msg->actual_mbx and submitting it to the hardware, the
cancellation attempt will fail because the message is no longer on the list.
The waiting thread will then read the transient -1 value for actual_mbx.

Could this cause the waiter to completely skip issuing the abort command and
proceed to free its DMA buffers after the 5-second wait? If the transaction
manager thread later wakes up and submits the request to the hardware, the
hardware might write crypto output directly into the freed DMA buffers,
causing a memory corruption regression.

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