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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 10 potential issue(s) to consider:
- [Critical] The synchronous submission timeout handler suffers from a TOCTOU race allowing Use-After-Free, and an unconditional abort that corrupts unrelated transactions.
- [Critical] Error handling for multi-VCQ transactions causes DMA Use-After-Free by completing the transaction early while the hardware continues executing its remaining VCQs.
- [Critical] Concurrent read-modify-write of the `R_MBX_QUEUE_TAIL` hardware register overwrites newly submitted VCQs.
- [Critical] Mailbox `FLUSH` commands silently discard in-flight transactions, which the Response Handler falsely completes with SUCCESS.
- [Critical] A TOCTOU list manipulation race between the TM kthread and the sync submit cancellation path allows hardware to DMA into caller buffers after they have been freed.
- [Critical] The DMA scatter-gather descriptor array is allocated with `kzalloc`, which does not guarantee the 64-byte alignment required by the hardware.
- [High] Head-of-line blocking vulnerability in the Transaction Manager thread when submitting an oversized transaction.
- [High] The `rh_watchdog` timer re-arms itself during teardown because `timer_delete_sync` is called before clearing the `rh.active` flag.
- [High] Completion callbacks are invoked with the internal `rh_process_lock` spinlock held, risking AB-BA deadlocks with external subsystem locks.
- [High] A TOCTOU race in `cmh_tm_post_command()` checking the `tm.running` flag outside of the queue lock allows messages to be leaked during teardown.
--

commit a1ed650ae307bbc8b4d1ebd0580bf2977503fc8f
Author: Alex Ousherovitch <aousherovitch@rambus.com>
Subject: crypto: cmh - add core platform driver

Adds the Rambus CryptoManager Hub (CMH) hardware crypto accelerator core
platform driver. This includes platform registration, a Virtual Command Queue
(VCQ) interface via mailbox, transaction manager, response handler, and
sysfs/debugfs integration.

> --- a/drivers/crypto/cmh/cmh_dma.c
> +++ b/drivers/crypto/cmh/cmh_dma.c

[ ... ]

> @@ -250,7 +250,7 @@ struct cmh_sg_map *cmh_dma_build_sg(const struct cmh_dma_buf *bufs, u32 count,
>  	/*
>  	 * Allocate descriptor array with kmalloc and map for streaming DMA.
>  	 * We map first to obtain items_dma (needed for .lli pointers),
>  	 * then sync-for-cpu, fill descriptors, and sync-for-device.
>  	 */
>  	sgm->items = kzalloc(sgm->items_size, gfp);

[Severity: Critical]
Is it possible this allocation could cause a hardware DMA fault? 

The hardware LLI format strictly requires 64-byte aligned descriptor chain
pointers, but kzalloc() only guarantees the architecture's minimum DMA
alignment (typically 8 or 16 bytes). Small allocations here might not satisfy
the 64-byte requirement.

> --- a/drivers/crypto/cmh/cmh_rh.c
> +++ b/drivers/crypto/cmh/cmh_rh.c

[ ... ]

> @@ -152,5 +152,5 @@ static unsigned int watchdog_ms = CMH_RH_WATCHDOG_MS_DEFAULT;
>  static void cmh_rh_poke_tail(void __iomem *base)
>  {
>  	u32 tail = cmh_reg_read32(base, R_MBX_QUEUE_TAIL);
>  
>  	cmh_reg_write32(tail, base, R_MBX_QUEUE_TAIL);
>  }

[Severity: Critical]
Could this sequence corrupt the hardware ring buffer? 

If cmh_rh_poke_tail() runs concurrently from softirq/watchdog contexts and
interleaves between submit_vcq()'s read and write of R_MBX_QUEUE_TAIL, it
might read the old TAIL and overwrite the newly advanced TAIL written by
submit_vcq(). 

This would roll back the hardware queue state and cause the next submission to
overwrite unexecuted VCQs.

[ ... ]

> @@ -395,5 +395,5 @@ static void cmh_rh_process_mbx(u32 mbx_idx, u32 new_head, u32 irq_bits)
>  		/* Now pop and fail the transaction that actually errored */
>  		txn = cmh_tm_pop_transaction(mbx_idx);
>  		if (txn) {
>  			dev_dbg(cmh_dev(), "rh: mbx[%u] error-complete vcq=%u..%u\n",
>  				mbx_idx, txn->first_vcq_id,
>  				txn->last_vcq_id);
>  			cmh_txn_finish(txn, error);
>  			cmh_tm_txq_completion_notify();
>  		} else {

[Severity: Critical]
If a multi-VCQ transaction encounters an error on a middle VCQ, could
completing the transaction here cause a use-after-free? 

When cmh_txn_finish() is called, the caller will unmap and free its DMA memory
buffers while the hardware might still have subsequent VCQs left to process.

[ ... ]

> @@ -420,2 +420,2 @@ static void cmh_rh_process_mbx(u32 mbx_idx, u32 new_head, u32 irq_bits)
>  		cmh_reg_write32(MBX_IRQ_MASK, base, R_MBX_INTERRUPT);
>  		cmh_reg_write32(MBX_COMMAND_RESTART, base, R_MBX_COMMAND);

[Severity: Critical]
Following up on the early completion above, does issuing MBX_COMMAND_RESTART
here force the hardware to resume execution of the remaining VCQs from the
failed transaction? 

If the DMA buffers were already freed by the caller, this would lead to memory
corruption when the hardware processes them.

[ ... ]

> @@ -435,5 +435,5 @@ static void cmh_rh_process_mbx(u32 mbx_idx, u32 new_head, u32 irq_bits)
>  	while ((txn = cmh_tm_peek_transaction(mbx_idx)) != NULL) {
>  		if ((s32)(new_head - txn->last_vcq_id) <= 0) {
>  			/*
>  			 * Not yet complete.  An intermediate HEAD within a
>  			 * multi-VCQ group is normal (the eSW advances HEAD
>  			 * per VCQ), so log at debug level and wait for the
>  			 * group to finish.
>  			 */
>  			if (txn->first_vcq_id != txn->last_vcq_id &&
>  			    (s32)(new_head - txn->first_vcq_id) > 0)
>  				dev_dbg_ratelimited(cmh_dev(),
>  						    "rh: mbx[%u] head %u mid-group %u..%u\n",
>  						    mbx_idx, new_head,
>  						    txn->first_vcq_id,
>  						    txn->last_vcq_id);
>  			break;
>  		}
>  
>  		txn = cmh_tm_pop_transaction(mbx_idx);
>  		if (!txn)
>  			break;
>  
>  		dev_dbg(cmh_dev(), "rh: mbx[%u] complete vcq=%u..%u err=%d\n",
>  			mbx_idx, txn->first_vcq_id, txn->last_vcq_id,
>  			 error);
>  
>  		cmh_rh_stat_add_completed(mbx_idx, txn->first_vcq_id,
>  					  txn->last_vcq_id);
>  
>  		cmh_txn_finish(txn, error);
>  		cmh_tm_txq_completion_notify();
>  	}

[Severity: Critical]
When a MBX_COMMAND_FLUSH command forces the hardware to discard the queue and
set HEAD = TAIL, won't this loop misinterpret the discarded transactions as
successful completions? 

This would return garbage data to the caller instead of appropriately failing
the aborted requests.

[ ... ]

> @@ -446,5 +446,5 @@ static irqreturn_t cmh_rh_thread(int irq, void *data)
>  
>  			spin_lock_bh(&rh_process_lock);
>  			new_head = cmh_reg_read32(cfg->mailboxes[i].reg_base,
>  						  R_MBX_QUEUE_HEAD);
>  
>  			if (new_head == rh.mbx[i].last_head && !irq_bits) {
>  				spin_unlock_bh(&rh_process_lock);
>  				continue;
>  			}
>  
>  			cmh_rh_process_mbx(i, new_head, irq_bits);
>  			rh.mbx[i].last_head = new_head;
>  			spin_unlock_bh(&rh_process_lock);
>  		}

[Severity: High]
Since cmh_rh_process_mbx() eventually invokes the caller-provided completion
callback via cmh_txn_finish(), does holding rh_process_lock here risk an AB-BA
deadlock? 

If the crypto API callback acquires an external lock, and another thread
holding that same lock calls into the driver (e.g. executing a timeout path
that calls cmh_rh_abort_mbx()), they could deadlock.

[ ... ]

> @@ -1023,7 +1023,7 @@ void cmh_rh_suspend(struct cmh_config *cfg)
>  {
>  	u32 i;
>  
>  	if (!rh.active)
>  		return;
>  
>  	/* Stop the watchdog before masking HW interrupts */
>  	timer_delete_sync(&rh_watchdog);
>  
>  	/* Mask MBX interrupts at the hardware level */
>  	for (i = 0; i < cfg->mbx_count; i++)
>  		cmh_reg_write32(0, cfg->mailboxes[i].reg_base,
>  				R_MBX_INTERRUPT_MASK);
>  
>  	/*
>  	 * Ensure no threaded IRQ handler is still in-flight.
>  	 * After masking, a handler may already have been scheduled.
>  	 * synchronize_irq() waits for it to complete before we
>  	 * proceed with suspend (which tears down TM state).
>  	 */
>  	for (i = 0; i < rh.nirqs; i++)
>  		synchronize_irq(rh.irqs[i]);
>  
>  	rh.active = false;
>  }

[Severity: High]
Does calling timer_delete_sync() before setting rh.active = false open a
window for the timer to incorrectly re-arm itself? 

If cmh_rh_watchdog_fn() is spinning concurrently on another CPU, it will
complete, see rh.active is still true, and successfully call mod_timer(). This
re-armed timer could later fire on freed memory. 

The same pattern exists in cmh_rh_cleanup().

> --- a/drivers/crypto/cmh/cmh_txn.c
> +++ b/drivers/crypto/cmh/cmh_txn.c

[ ... ]

> @@ -611,6 +611,6 @@ static int select_mailbox(u32 slots_needed)
>  {
>  	u32 count = tm.cfg->mbx_count;
>  	u32 start = tm.next_mbx;
>  	u32 i;
>  
>  	for (i = 0; i < count; i++) {
>  		u32 idx = (start + i) % count;
>  
>  		if (cmh_rh_mbx_is_wedged(idx))
>  			continue;
>  
>  		if (mbx_free_slots(&tm.cfg->mailboxes[idx]) >= slots_needed) {
>  			tm.next_mbx = (idx + 1) % count;
>  			return (int)idx;
>  		}

[Severity: High]
If a request is submitted with a slots_needed value that exceeds the absolute
maximum capacity of the mailbox ring buffer, won't this condition permanently
fail?

[ ... ]

> @@ -829,3 +829,3 @@ static int cmh_tm_thread(void *data)
>  			spin_lock_irqsave(&tm.cmq_lock, flags);
>  			list_add(&msg->list, &tm.cmq);
>  			cmq_depth++;
>  			spin_unlock_irqrestore(&tm.cmq_lock, flags);
>  
>  			tmo = usecs_to_jiffies(CMH_TM_BACKOFF_MAX_US);
>  			wait_event_interruptible_timeout(tm.cmq_waitq,
>  							 kthread_should_stop() ||
>  							 atomic_read(&txq_completion_gen) != gen,
>  							 tmo ?: 1);
>  			cmh_stat_inc_tm_backoff();
>  			continue;
>  		}

[Severity: High]
If the message failed to resolve a mailbox because it requested more VCQs than
the queue can ever hold, putting it back at the head of tm.cmq will cause the
kthread to retry it endlessly. 

Could this create a head-of-line blocking livelock that stalls all other
crypto operations on the system?

[ ... ]

> @@ -1238,6 +1238,6 @@ bool cmh_tm_try_cancel_command(struct command_msg *msg)
>  {
>  	unsigned long flags;
>  	bool cancelled = false;
>  
>  	spin_lock_irqsave(&tm.cmq_lock, flags);
>  	if (!list_empty(&msg->list)) {
>  		list_del_init(&msg->list);
>  		cmq_depth--;
>  		cancelled = true;
>  	}
>  	spin_unlock_irqrestore(&tm.cmq_lock, flags);
>  
>  	return cancelled;
>  }

[Severity: Critical]
Could this check return false incorrectly if cmh_tm_try_cancel_command() runs
exactly when the TM thread has temporarily dequeued the message using
list_del_init() to resolve a mailbox? 

If the cancellation fails here, the caller might drop references and free DMA
buffers, but the TM thread could subsequently re-add or submit the request,
causing the hardware to DMA into freed memory.

[ ... ]

> @@ -1266,4 +1266,4 @@ int cmh_tm_post_command(struct command_msg *msg)
>  {
>  	unsigned long flags;
>  
>  	if (!READ_ONCE(tm.running))
>  		return -ENODEV;
>  
>  	spin_lock_irqsave(&tm.cmq_lock, flags);

[Severity: High]
Is there a window here where a thread could check the tm.running flag, get
preempted, and resume after cmh_tm_cleanup() has already stopped the TM thread
and drained the queue? 

The preempted thread would then acquire the lock and enqueue its message into
a dead queue, leaking the memory and potentially hanging the caller
indefinitely.

[ ... ]

> @@ -1453,7 +1453,7 @@ int cmh_tm_submit_sync_tmo(struct vcq_cmd *vcq_cmds, u32 vcq_count,
>  			 * rh_process_lock, preventing clobber of a
>  			 * concurrent RESTART/FLUSH from the watchdog.
>  			 */
>  			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: Critical]
Is it safe to issue an unconditional MBX_COMMAND_ABORT here based solely on
msg->actual_mbx? 

If the TM thread dequeued the request but hasn't updated actual_mbx yet, this
skips the abort entirely, returning -ETIMEDOUT and letting the caller free DMA
buffers while the hardware continues executing. 

Conversely, if actual_mbx is set but this transaction has already completed,
issuing an abort might spuriously terminate a completely unrelated transaction
currently executing in the hardware.

[ ... ]

> @@ -1609,4 +1609,4 @@ int cmh_tm_submit_async(struct vcq_cmd *vcq_cmds, u32 vcq_count,
>  	msg->command_id      = vcq_cmds[1].id;
>  	msg->vcq_count       = vcq_count;
>  	msg->num_vcqs        = num_vcqs;
>  	msg->target_mbx      = target_mbx;

[Severity: High]
Should there be validation here to reject the request if num_vcqs is larger
than the maximum capacity of any mailbox? 

If an impossibly large num_vcqs is accepted, it triggers the head-of-line
blocking livelock described above.

[ ... ]

> @@ -1836,4 +1836,4 @@ int cmh_tm_flush_mbx(s32 mbx_idx)
>  	/* Ensure no command is already pending */
>  	if (cmh_reg_read32(base, R_MBX_COMMAND) != 0) {
>  		mutex_unlock(&txq->dispatch_lock);
>  		return -EBUSY;
>  	}
>  
>  	cmh_reg_write32(MBX_COMMAND_FLUSH, base, R_MBX_COMMAND);

[Severity: Critical]
By issuing MBX_COMMAND_FLUSH here without verifying if the software
transaction queue is empty (e.g. checking txq->depth == 0), aren't actively
running VCQs silently destroyed? 

Since submit_vcq() only updates TAIL and leaves COMMAND as 0, this check
incorrectly assumes the mailbox is idle.

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