Re: [PATCH 2/3] scsi: arcmsr: check the completion index the adapter reports

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] scsi: arcmsr: break in arcmsr_hbaE_postqueue_isr() causes infinite loop
- [High] scsi: arcmsr: incomplete bounds check allows OOB access if maxFreeCCB > 1024
- [High] scsi: arcmsr: stale cmdSMID left by polling causes double completion on Type F
--

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

scsi: arcmsr: check the completion index the adapter reports

This commit adds bounds checking for the cmdSMID read from the completion
queue on adapter types E and F. It prevents out-of-bounds array indexing
when the adapter reports invalid or out-of-range slot numbers.

> diff --git a/drivers/scsi/arcmsr/arcmsr_hba.c b/drivers/scsi/arcmsr/arcmsr_hba.c
> index 255f1d898079f..1381420d9fbd7 100644
> --- a/drivers/scsi/arcmsr/arcmsr_hba.c
> +++ b/drivers/scsi/arcmsr/arcmsr_hba.c

[ ... ]

> @@ -2677,6 +2677,11 @@ static void arcmsr_hbaE_postqueue_isr(struct AdapterControlBlock *acb)
>  	pmu = acb->pmuE;
>  	while ((readl(&pmu->reply_post_producer_index) & 0xFFFF) != doneq_index) {
>  		cmdSMID = acb->pCompletionQ[doneq_index].cmdSMID;
> +		if (cmdSMID >= acb->maxFreeCCB) {
> +			pr_err_ratelimited("arcmsr%d: bad cmdSMID %u from iop\n",
> +					   acb->host->host_no, cmdSMID);
> +			break;
> +		}

[Severity: High]
Does this code risk causing an infinite interrupt loop?

By breaking out of the loop early, the function exits without incrementing
doneq_index. As a result, the unincremented doneq_index is written to the
adapter's consumer index register. 

Because the consumer index is not advanced past the bad entry, the hardware
continues to assert the postqueue interrupt. When the ISR returns, the
interrupt remains asserted, causing an immediate IRQ storm that loops
infinitely on the same invalid entry.

Should the driver instead advance the consumer index, clear the entry if
necessary, and use continue?

[Severity: High]
Can this still overflow acb->pccb_pool if acb->maxFreeCCB exceeds 1024?

If the user sets the host_can_queue module parameter between 961 and 1023,
arcmsr_get_firmware_spec() executes:

    if (acb->maxFreeCCB < ARCMSR_MAX_FREECCB_NUM)
        acb->maxFreeCCB += 64;

This can result in acb->maxFreeCCB being greater than ARCMSR_MAX_FREECCB_NUM
(1024).

If the adapter reports a cmdSMID that is between 1024 and maxFreeCCB, it
passes this check but still overflows the statically sized 1024-element
acb->pccb_pool array.

Should the check use ARRAY_SIZE(acb->pccb_pool) or ARCMSR_MAX_FREECCB_NUM
instead of maxFreeCCB?

>  		ccb = acb->pccb_pool[cmdSMID];
>  		error = (acb->pCompletionQ[doneq_index].cmdFlag
> 			& ARCMSR_CCBREPLY_FLAG_ERROR_MODE1) ? true : false;

[ ... ]

> @@ -2706,6 +2711,11 @@ static void arcmsr_hbaF_postqueue_isr(struct AdapterControlBlock *acb)
>  		cmdSMID = acb->pCompletionQ[doneq_index].cmdSMID;
>  		if (cmdSMID == 0xffff)
>  			break;
> +		if (cmdSMID >= acb->maxFreeCCB) {
> +			pr_err_ratelimited("arcmsr%d: bad cmdSMID %u from iop\n",
> +					   acb->host->host_no, cmdSMID);
> +			break;
> +		}

[Severity: High]
Does breaking here prevent doneq_index from advancing and clearing the queue
entry?

Type F adapters rely on the queue entry being cleared to 0xffff. By breaking,
the entry is never cleared, which will permanently stall the completion queue.

[Severity: High]
Does this bounds check share the same issue with maxFreeCCB exceeding 1024 as
mentioned above?

>  		ccb = acb->pccb_pool[cmdSMID];
>  		error = (acb->pCompletionQ[doneq_index].cmdFlag &

[ ... ]

> @@ -3880,6 +3890,11 @@ static int arcmsr_hbaE_polling_ccbdone(struct AdapterControlBlock *acb,
>  			doneq_index = 0;
>  		acb->doneq_index = doneq_index;
>  		spin_unlock_irqrestore(&acb->doneq_lock, flags);
> +		if (cmdSMID >= acb->maxFreeCCB) {
> +			pr_err_ratelimited("arcmsr%d: bad cmdSMID %u from iop\n",
> +					   acb->host->host_no, cmdSMID);
> +			continue;
> +		}

[Severity: High]
Does this bounds check also suffer from the maxFreeCCB overflow issue?

[Severity: High]
Does this code risk leaving a stale cmdSMID that poisons the ring buffer for
Type F adapters?

When a SCSI command times out, arcmsr_abort() uses this function (which serves
both Type E and Type F adapters) to poll completions. 

It successfully processes completions and advances doneq_index:

    cmdSMID = acb->pCompletionQ[doneq_index].cmdSMID;
    doneq_index++;
    if (doneq_index >= acb->completionQ_entry)
        doneq_index = 0;
    acb->doneq_index = doneq_index;
    spin_unlock_irqrestore(&acb->doneq_lock, flags);

However, it fails to clear cmdSMID to 0xffff after consuming the entry.

Later, as normal I/O continues and doneq_index wraps around,
arcmsr_hbaF_postqueue_isr() (which strictly relies on checking
cmdSMID == 0xffff to find empty slots) will encounter the uncleared cmdSMID
and misinterpret it as a new completion. 

Can this lead to double CCB completions, list corruption, or use-after-free
if the CCB has already been reused?

>  		pCCB = acb->pccb_pool[cmdSMID];
>  		poll_ccb_done |= (pCCB == poll_ccb) ? 1 : 0;

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