Re: [PATCH v2 3/3] scsi: arcmsr: wrap the type B completion index before using it

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] scsi: arcmsr: missing lock in arcmsr_hbaB_postqueue_isr() allows double scsi_done
- [High] scsi: arcmsr: missing bounds check on device-supplied flag_ccb pointer arithmetic
--

commit d4c56c066e7826c7ed7b29fee09fa6ea641b3ec5
Author: Yehyeong Lee <yhlee@isslab.korea.ac.kr>

scsi: arcmsr: wrap the type B completion index before using it

This commit fixes an out-of-bounds read in arcmsr_hbaB_postqueue_isr()
and arcmsr_hbaB_polling_ccbdone() by applying a modulo to doneq_index 
before using it as an array index. The index is read from shared memory 
where it could exceed the maximum queue size before being wrapped.

Cc: stable@vger.kernel.org

> --- a/drivers/scsi/arcmsr/arcmsr_hba.c
> +++ b/drivers/scsi/arcmsr/arcmsr_hba.c
> @@ -2568,7 +2568,7 @@ static void arcmsr_hbaB_postqueue_isr(struct AdapterControlBlock *acb)
>  	bool error;
>  	unsigned long cdb_phy_addr;
>  
> -	index = reg->doneq_index;
> +	index = reg->doneq_index % ARCMSR_MAX_HBB_POSTQUEUE;
>  	while ((flag_ccb = reg->done_qbuffer[index]) != 0) {

[Severity: High]
Does this loop need synchronization to protect the shared completion ring?

The SCSI error handler invokes arcmsr_abort() without holding the host_lock,
which in turn calls arcmsr_hbaB_polling_ccbdone(). Neither
arcmsr_hbaB_postqueue_isr() nor arcmsr_hbaB_polling_ccbdone() acquires
acb->doneq_lock (unlike adapters D and F) or any other synchronization
primitive before reading from the hardware completion ring.

If a command times out and arcmsr_abort() executes on CPU 1 just as the
device interrupt fires and executes arcmsr_hbaB_postqueue_isr() on CPU 2,
could both threads fetch the exact same CCB completion entry? This could
result in concurrently invoking arcmsr_ccb_complete(), leading to a double
scsi_dma_unmap(), double scsi_done(), and corruption of the ccb_free_list.

>  		cdb_phy_addr = (flag_ccb << 5) & 0xffffffff;
>  		if (acb->cdb_phyadd_hipart)

[Severity: High]
Can a compromised or malicious hardware device supply a crafted flag_ccb to
trigger arbitrary kernel memory corruption?

The driver derives the pCCB kernel virtual pointer directly from the
device-supplied flag_ccb index without ensuring it falls within the driver's
preallocated CCB pool. When the driver processes this pointer in
arcmsr_drain_donequeue() or arcmsr_report_ccb_state(), it might dereference
an attacker-controlled pointer and write values like DID_OK << 16 to it.

Should there be a software-based bounds check on the calculated offset
before converting it into a CommandControlBlock pointer to protect against
untrusted device inputs?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1791792888.git.yhlee@isslab.korea.ac.kr?part=3