[PATCH v2 0/3] scsi: arcmsr: bound adapter-supplied completion indices and message lengths

[PATCH v2 0/3] scsi: arcmsr: bound adapter-supplied completion indices and message lengths

[Yehyeong Lee]

The arcmsr completion and message paths take several values straight
from the DMA area the adapter writes -- a completion slot number, a
type B completion-ring index, and an inbound message length -- and use
them as array subscripts or copy lengths without checking them against
the array or buffer they index.  A malfunctioning or malicious adapter
can therefore drive an out-of-bounds access from the completion
interrupt or an ioctl.

Patch 1 clamps the firmware-supplied message length to the staging
window (heap out-of-bounds write).  Patch 2 rejects a completion slot
the driver never handed out (wild pointer load and dereference on types
E and F).  Patch 3 wraps the type B completion index before it is used
(out-of-bounds read, and an out-of-bounds store when the word read is
non-zero).

Each was reproduced on a KASAN build with a crafted DMA area; the
per-patch changelogs have the reports.  The three are independent and
carry separate Fixes tags.

v1: https://lore.kernel.org/linux-scsi/cover.1791789900.git.yhlee@isslab.korea.ac.kr/

Changes since v1 (patches 1 and 3 unchanged):
 - patch 2: check the completion index against ARCMSR_MAX_FREECCB_NUM
   (the pccb_pool size) rather than acb->maxFreeCCB, which
   arcmsr_get_firmware_spec() can raise above it; and skip a bad entry
   and continue rather than break, so the completion consumer index
   still advances (clearing the entry to 0xffff on type F), instead of
   stalling the queue.

Yehyeong Lee (3):
  scsi: arcmsr: bound the firmware-supplied message length
  scsi: arcmsr: check the completion index the adapter reports
  scsi: arcmsr: wrap the type B completion index before using it

 drivers/scsi/arcmsr/arcmsr_hba.c | 30 ++++++++++++++++++++++++++++--
 1 file changed, 28 insertions(+), 2 deletions(-)

-- 
2.43.0

[PATCH v2 1/3] scsi: arcmsr: bound the firmware-supplied message length

[Yehyeong Lee]

The adapter hands the driver an inbound message through a fixed
128-byte window:

	struct QBUFFER {
		uint32_t	data_len;
		uint8_t		data[124];
	};

arcmsr_Read_iop_rqbuffer_in_DWORD() takes data_len from that window and
uses it as the copy length without comparing it against the window:

	iop_len = readl(&prbuffer->data_len);
	if (iop_len > 0) {
		buf1 = kmalloc(128, GFP_ATOMIC);
		...
		data_len = iop_len;
		while (data_len >= 4) {
			*buf2++ = readl(iop_data);
			iop_data++;
			data_len -= 4;
		}

A data_len above 128 therefore walks the 128-byte allocation, writing
firmware-supplied data past its end, and the loop that follows reads the
same overrun back out of buf1.  arcmsr_Read_iop_rqbuffer_data() has the
same unchecked loop for adapter types A and B; there the destination is
wrapped to ARCMSR_MAX_QBUFFER so nothing is written out of bounds, but
the loop still spins up to 2^32 times with rqbuffer_lock held.

Two of the three callers place no bound on the value at all:
arcmsr_iop_message_xfer() for ARCMSR_MESSAGE_READ_RQBUFFER and
arcmsr_sysfs_iop_message_read() both call straight into the reader on
ACB_F_IOPDATA_OVERFLOW.  The third, arcmsr_iop2drv_data_wrote_handle(),
compares data_len against the free space in acb->rqbuffer, which is up
to ARCMSR_MAX_QBUFFER - 1 and so still far larger than the 128-byte
staging buffer; it also reads data_len separately from the reader, so
the adapter can change it between the two reads.

Clamp the length to the size of the window in both readers.  A
conforming adapter never reports more than sizeof(prbuffer->data), so
this does not drop any data it sends.

With a KASAN build and a crafted window, data_len = 4096 produces 992
out-of-bounds 4-byte writes (1024 dwords less the 32 that fit) in
arcmsr_Read_iop_rqbuffer_in_DWORD():

  BUG: KASAN: slab-out-of-bounds in arcmsr_Read_iop_rqbuffer_in_DWORD+0x240/0x2a0 [arcmsr]
  Write of size 4 at addr ffff88800b5baf80 by task init/1
  Call Trace:
   <TASK>
   dump_stack_lvl+0x66/0xa0
   print_report+0xd0/0x630
   kasan_report+0xe4/0x120
   arcmsr_Read_iop_rqbuffer_in_DWORD+0x240/0x2a0 [arcmsr]
   </TASK>
  Allocated by task 1:
   __kasan_kmalloc+0x8f/0xa0
   __kmalloc_cache_noprof+0x22c/0x570
   arcmsr_Read_iop_rqbuffer_in_DWORD+0x7b/0x2a0 [arcmsr]
  The buggy address belongs to the object at ffff88800b5baf00
   which belongs to the cache kmalloc-128 of size 128
  The buggy address is located 0 bytes to the right of

The first value that overruns is data_len = 129; 128 and below are
reported clean, and 124 (the window) and 120 copy byte-for-byte
identically with and without the clamp.

Fixes: bb263c4ecbb1 ("arcmsr: fix ioctl data read/write error for adapter type C")
Cc: stable@vger.kernel.org
Signed-off-by: Yehyeong Lee <yhlee@isslab.korea.ac.kr>
---
 drivers/scsi/arcmsr/arcmsr_hba.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/scsi/arcmsr/arcmsr_hba.c b/drivers/scsi/arcmsr/arcmsr_hba.c
index 8aa948f06cacf..255f1d898079f 100644
--- a/drivers/scsi/arcmsr/arcmsr_hba.c
+++ b/drivers/scsi/arcmsr/arcmsr_hba.c
@@ -2277,6 +2277,8 @@ arcmsr_Read_iop_rqbuffer_in_DWORD(struct AdapterControlBlock *acb,
 
 	iop_data = (uint32_t __iomem *)prbuffer->data;
 	iop_len = readl(&prbuffer->data_len);
+	if (iop_len > sizeof(prbuffer->data))
+		iop_len = sizeof(prbuffer->data);
 	if (iop_len > 0) {
 		buf1 = kmalloc(128, GFP_ATOMIC);
 		buf2 = (uint32_t *)buf1;
@@ -2319,6 +2321,8 @@ arcmsr_Read_iop_rqbuffer_data(struct AdapterControlBlock *acb,
 		return arcmsr_Read_iop_rqbuffer_in_DWORD(acb, prbuffer);
 	iop_data = (uint8_t __iomem *)prbuffer->data;
 	iop_len = readl(&prbuffer->data_len);
+	if (iop_len > sizeof(prbuffer->data))
+		iop_len = sizeof(prbuffer->data);
 	while (iop_len > 0) {
 		pQbuffer = &acb->rqbuffer[acb->rqbuf_putIndex];
 		*pQbuffer = readb(iop_data);
-- 
2.43.0

Re: [PATCH v2 1/3] scsi: arcmsr: bound the firmware-supplied message length

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] scsi: arcmsr: TOCTOU on prbuffer->data_len allows ring buffer corruption
--

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

scsi: arcmsr: bound the firmware-supplied message length

This commit bounds the firmware-supplied message length to prevent out-of-bounds
writes. It clamps the length of inbound messages to the 128-byte window size
during reads from the adapter. This fixes an issue where a malicious or
misbehaving firmware could specify an oversized data length and overrun the
allocated buffer.

>     The adapter hands the driver an inbound message through a fixed
>     128-byte window:
>     
>             struct QBUFFER {
>                     uint32_t        data_len;
>                     uint8_t         data[124];
>             };
>     
>     arcmsr_Read_iop_rqbuffer_in_DWORD() takes data_len from that window and
>     uses it as the copy length without comparing it against the window:

[ ... ]

>     Two of the three callers place no bound on the value at all:
>     arcmsr_iop_message_xfer() for ARCMSR_MESSAGE_READ_RQBUFFER and
>     arcmsr_sysfs_iop_message_read() both call straight into the reader on
>     ACB_F_IOPDATA_OVERFLOW.  The third, arcmsr_iop2drv_data_wrote_handle(),
>     compares data_len against the free space in acb->rqbuffer, which is up
>     to ARCMSR_MAX_QBUFFER - 1 and so still far larger than the 128-byte
>     staging buffer; it also reads data_len separately from the reader, so
>     the adapter can change it between the two reads.
>     
>     Clamp the length to the size of the window in both readers.  A
>     conforming adapter never reports more than sizeof(prbuffer->data), so
>     this does not drop any data it sends.

[ ... ]

> diff --git a/drivers/scsi/arcmsr/arcmsr_hba.c b/drivers/scsi/arcmsr/arcmsr_hba.c
> index 8aa948f06cacf..255f1d898079f 100644
> --- a/drivers/scsi/arcmsr/arcmsr_hba.c
> +++ b/drivers/scsi/arcmsr/arcmsr_hba.c
> @@ -2277,6 +2277,8 @@ arcmsr_Read_iop_rqbuffer_in_DWORD(struct AdapterControlBlock *acb,
>  
>  	iop_data = (uint32_t __iomem *)prbuffer->data;
>  	iop_len = readl(&prbuffer->data_len);
> +	if (iop_len > sizeof(prbuffer->data))
> +		iop_len = sizeof(prbuffer->data);

[Severity: Medium]
This problem wasn't introduced by this patch, but as noted in the commit
message, the adapter can change data_len between reads. Can this still lead
to ring buffer corruption?

In drivers/scsi/arcmsr/arcmsr_hba.c:arcmsr_iop2drv_data_wrote_handle(),
the first fetch checks available space:

	if (buf_empty_len >= readl(&prbuffer->data_len)) {
		if (arcmsr_Read_iop_rqbuffer_data(acb, prbuffer) == 0)

Then here in arcmsr_Read_iop_rqbuffer_in_DWORD() (and similarly in
arcmsr_Read_iop_rqbuffer_data()), data_len is fetched again directly from
MMIO.

If the firmware changes the value from a small size (e.g., 4) to 124 between
the two reads, it bypasses the buf_empty_len check. Even though this patch
clamps the length to sizeof(prbuffer->data), it can still write up to 124 bytes
into a ring buffer that lacks sufficient space.

This advances acb->rqbuf_putIndex past acb->rqbuf_getIndex. Because the ring
buffer calculation relies on the difference between these pointers, does this
wrap cause previously unread messages to be silently dropped and corrupt the
message stream to userspace?

Could the length be read once in arcmsr_iop2drv_data_wrote_handle() and passed
down to the read functions to avoid the double fetch?

>  	if (iop_len > 0) {
>  		buf1 = kmalloc(128, GFP_ATOMIC);
>  		buf2 = (uint32_t *)buf1;
> @@ -2319,6 +2321,8 @@ arcmsr_Read_iop_rqbuffer_data(struct AdapterControlBlock *acb,
>  		return arcmsr_Read_iop_rqbuffer_in_DWORD(acb, prbuffer);
>  	iop_data = (uint8_t __iomem *)prbuffer->data;
>  	iop_len = readl(&prbuffer->data_len);
> +	if (iop_len > sizeof(prbuffer->data))
> +		iop_len = sizeof(prbuffer->data);
>  	while (iop_len > 0) {
>  		pQbuffer = &acb->rqbuffer[acb->rqbuf_putIndex];
>  		*pQbuffer = readb(iop_data);

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

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

[Yehyeong Lee]

On adapter types E and F the adapter reports a completed command by
writing its slot number into the completion queue, and the driver turns
that number straight into an array subscript:

	cmdSMID = acb->pCompletionQ[doneq_index].cmdSMID;
	ccb = acb->pccb_pool[cmdSMID];

cmdSMID is a uint16_t taken from DMA memory the adapter writes, so it
can be anything up to 65535, while acb->pccb_pool[] has
ARCMSR_MAX_FREECCB_NUM (1024) entries.  Nothing compares the two.

arcmsr_drain_donequeue() cannot stand in for the missing check: it
starts by reading pCCB->acb and pCCB->startdone, so by the time it can
notice anything the bad pointer has already been loaded and
dereferenced.  arcmsr_hbaF_postqueue_isr() rejects the single value
0xffff because that is its queue-empty sentinel, which leaves
1024..65534 accepted.

Reject an index the driver never handed out, in the two interrupt
handlers and in the polling path.

Bound against ARCMSR_MAX_FREECCB_NUM rather than acb->maxFreeCCB, which
is what actually guards the array: maxFreeCCB is derived from the
host_can_queue module parameter and gets 64 added to it when it is below
ARCMSR_MAX_FREECCB_NUM, so it can be larger than the array and would let
a slot number past the end through.

In the interrupt handlers, release the entry and step the queue index
before continuing, the same way the accepted path does.  Stopping at the
bad entry instead would leave the consumer index behind the producer,
so the adapter would re-raise the interrupt on an entry the driver has
already refused and the completions queued behind it would never be
reaped.

Measured with a KASAN build and a crafted completion queue.  struct
AdapterControlBlock is 17504 bytes with pccb_pool at offset 496, so a
reported slot of 60000 reads 480496 bytes in, past the end of the
controller block entirely:

  BUG: KASAN: use-after-free in arcmsr_hbaE_postqueue_isr+0x242/0x2d0 [arcmsr]
  Read of size 8 at addr ffff88800bf354f0 by task init/1

and the pointer it picked up then faults in arcmsr_drain_donequeue:

  RIP: 0010:arcmsr_drain_donequeue+0x27/0x360 [arcmsr]
  Kernel panic - not syncing: Attempted to kill init! exitcode=0x0000000b

A smaller out-of-range slot such as 2000 still lands inside the
controller block (offset 16496), so there is nothing for KASAN to
report; the driver simply reads an unrelated field of its own
AdapterControlBlock as a command pointer and faults on it in the same
place.  With host_can_queue raised so that maxFreeCCB becomes 1087, a
reported slot of 1050 faults the same way while comparing below
maxFreeCCB.

A queue holding a rejected entry followed by a good one drains to the
producer index and the good completion is reaped; valid slot numbers are
accepted unchanged.

Fixes: 235090241c55 ("scsi: arcmsr: Add code for ACB_ADAPTER_TYPE_E")
Cc: stable@vger.kernel.org
Signed-off-by: Yehyeong Lee <yhlee@isslab.korea.ac.kr>
---
 drivers/scsi/arcmsr/arcmsr_hba.c | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

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) {
+			pr_err_ratelimited("arcmsr%d: bad cmdSMID %u from iop\n",
+					   acb->host->host_no, cmdSMID);
+			doneq_index++;
+			if (doneq_index >= acb->completionQ_entry)
+				doneq_index = 0;
+			continue;
+		}
 		ccb = acb->pccb_pool[cmdSMID];
 		error = (acb->pCompletionQ[doneq_index].cmdFlag
 			& ARCMSR_CCBREPLY_FLAG_ERROR_MODE1) ? true : false;
@@ -2706,6 +2714,15 @@ static void arcmsr_hbaF_postqueue_isr(struct AdapterControlBlock *acb)
 		cmdSMID = acb->pCompletionQ[doneq_index].cmdSMID;
 		if (cmdSMID == 0xffff)
 			break;
+		if (cmdSMID >= ARCMSR_MAX_FREECCB_NUM) {
+			pr_err_ratelimited("arcmsr%d: bad cmdSMID %u from iop\n",
+					   acb->host->host_no, cmdSMID);
+			acb->pCompletionQ[doneq_index].cmdSMID = 0xffff;
+			doneq_index++;
+			if (doneq_index >= acb->completionQ_entry)
+				doneq_index = 0;
+			continue;
+		}
 		ccb = acb->pccb_pool[cmdSMID];
 		error = (acb->pCompletionQ[doneq_index].cmdFlag &
 			ARCMSR_CCBREPLY_FLAG_ERROR_MODE1) ? true : false;
@@ -3880,6 +3897,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 >= ARCMSR_MAX_FREECCB_NUM) {
+			pr_err_ratelimited("arcmsr%d: bad cmdSMID %u from iop\n",
+					   acb->host->host_no, cmdSMID);
+			continue;
+		}
 		pCCB = acb->pccb_pool[cmdSMID];
 		poll_ccb_done |= (pCCB == poll_ccb) ? 1 : 0;
 		/* check if command done with no error*/
-- 
2.43.0

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

[sashiko-bot]

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

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

[Yehyeong Lee]

struct MessageUnit_B lives in the dma_alloc_coherent() area shared with
the adapter, and holds the completion ring next to the index into it:

	uint32_t	done_qbuffer[ARCMSR_MAX_HBB_POSTQUEUE];
	uint32_t	postq_index;
	uint32_t	doneq_index;

arcmsr_hbaB_postqueue_isr() takes doneq_index from there and subscripts
done_qbuffer[] with it, and only afterwards reduces it:

	index = reg->doneq_index;
	while ((flag_ccb = reg->done_qbuffer[index]) != 0) {
		...
		reg->done_qbuffer[index] = 0;
		index++;
		index %= ARCMSR_MAX_HBB_POSTQUEUE;
		reg->doneq_index = index;
	}

So the first iteration uses the value as it was found, unmasked.  An
index beyond ARCMSR_MAX_HBB_POSTQUEUE (264) reads past done_qbuffer[]
and, should the word it finds be non-zero, stores a zero there as well.
arcmsr_hbaB_polling_ccbdone() has the same ordering.

Reduce the index where it is read instead, which leaves the rest of the
loop unchanged.

Measured with a KASAN build.  struct MessageUnit_B is 2176 bytes with
done_qbuffer at offset 1056, so a reported index of 600 reads 3456 bytes
in:

  BUG: KASAN: slab-out-of-bounds in arcmsr_hbaB_postqueue_isr+0x1f5/0x270 [arcmsr]
  Read of size 4 at addr ffff88800c08cd80 by task init/1
  ...
   which belongs to the cache kmalloc-4k of size 4096
  The buggy address is located 1280 bytes to the right of

Fixes: 1c57e86d75cf ("[SCSI] arcmsr: initial driver, version 1.20.00.13")
Cc: stable@vger.kernel.org
Signed-off-by: Yehyeong Lee <yhlee@isslab.korea.ac.kr>
---
 drivers/scsi/arcmsr/arcmsr_hba.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/scsi/arcmsr/arcmsr_hba.c b/drivers/scsi/arcmsr/arcmsr_hba.c
index 9519715883b5a..c2c62ad6c6502 100644
--- 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) {
 		cdb_phy_addr = (flag_ccb << 5) & 0xffffffff;
 		if (acb->cdb_phyadd_hipart)
@@ -3669,7 +3669,7 @@ static int arcmsr_hbaB_polling_ccbdone(struct AdapterControlBlock *acb,
 	/* clear doorbell interrupt */
 	writel(ARCMSR_DOORBELL_INT_CLEAR_PATTERN, reg->iop2drv_doorbell);
 	while(1){
-		index = reg->doneq_index;
+		index = reg->doneq_index % ARCMSR_MAX_HBB_POSTQUEUE;
 		flag_ccb = reg->done_qbuffer[index];
 		if (flag_ccb == 0) {
 			if (poll_ccb_done){
-- 
2.43.0

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

[sashiko-bot]

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