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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Critical] scsi: arcmsr: incomplete bounds check allows NULL pointer dereference
--

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

scsi: arcmsr: check the completion index the adapter reports

This commit adds bounds checks in the interrupt and polling handlers for
adapter types E and F. It validates that the completion index reported by
the adapter does not exceed ARCMSR_MAX_FREECCB_NUM to prevent out-of-bounds
array accesses when retrieving command control blocks.

> diff --git a/drivers/scsi/arcmsr/arcmsr_hba.c b/drivers/scsi/arcmsr/arcmsr_hba.c
> index 255f1d898079f..9519715883b5a 100644
> --- a/drivers/scsi/arcmsr/arcmsr_hba.c
> +++ b/drivers/scsi/arcmsr/arcmsr_hba.c
> @@ -2677,6 +2677,14 @@ 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 >= ARCMSR_MAX_FREECCB_NUM) {

[Severity: Critical]
Is this bounds check tight enough?

The commit message notes the choice to bound against ARCMSR_MAX_FREECCB_NUM
(1024) instead of acb->maxFreeCCB. However, the acb->pccb_pool array is
only populated up to acb->maxFreeCCB (which defaults to 192).

This leaves array elements between acb->maxFreeCCB and 1023 uninitialized
and set to NULL. If a malfunctioning or malicious adapter sends a cmdSMID
in this gap (e.g., 200), it bypasses this bounds check.

The driver will then retrieve a NULL pointer for the CCB and blindly
dereference it in arcmsr_drain_donequeue():

drivers/scsi/arcmsr/arcmsr_hba.c:arcmsr_drain_donequeue()
    if ((pCCB->acb != acb) || (pCCB->startdone != ARCMSR_CCB_START)) {
        ...

Could this regression lead to a NULL pointer dereference panic? Would it
make sense to add a check to ensure the retrieved CCB pointer is not NULL
before passing it to arcmsr_drain_donequeue(), or enforce a stricter bound
that correctly handles the uninitialized pool entries?

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