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

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

[Yehyeong Lee]

arcmsr uses three values from the DMA area the adapter writes - a
completion slot number, a type B ring index and a message length - as
subscripts or copy lengths unchecked.  Patches 1-3 clamp the length,
reject an unissued slot and wrap the index, each reproduced on KASAN.

Patch 4 is not about an adapter-supplied value.  Type B is the only type
whose completion ring is consumed with no lock: D, E and F take
acb->doneq_lock on the interrupt and the polling side, and patch 4 makes
B match them.  arcmsr_abort() masks the outbound interrupts but never
waits for a running handler, so a working adapter is enough.

Not addressed.  flag_ccb is unchecked but is the CDB address the driver
posted echoed back, so only a fabricating adapter leaves the pool.  The
data_len double fetch needs the firmware to change the field between the
two reads, which patch 1 bounds anyway.  Type F polling not restoring
cmdSMID is also handled by the dedicated polling function in
https://lore.kernel.org/all/91fa9048-ae23-4813-a8fe-bd64d5b374d5@areca.com.tw/
so I am not sending it; I will rebase whichever lands second.

v4:
- Add patch 4, serialising the type B completion ring with doneq_lock,
  as raised in review of v2 3/3:
  https://lore.kernel.org/all/sashiko-outbox-167892@kernel.org/
- Patches 1-3 unchanged.

Best regards,
Yehyeong Lee

Yehyeong Lee (4):
  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
  scsi: arcmsr: serialise the type B completion ring

 drivers/scsi/arcmsr/arcmsr_hba.c | 45 +++++++++++++++++++++++++++++---
 1 file changed, 41 insertions(+), 4 deletions(-)


base-commit: 97d73c8251226cf27cc6656b4c542869bb8c69fa
-- 
2.43.0

[PATCH v4 1/4] 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 8aa948f06cacf1..255f1d898079f6 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

[PATCH v4 2/4] 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.  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 acb->maxFreeCCB rather than ARCMSR_MAX_FREECCB_NUM.  The
array has ARCMSR_MAX_FREECCB_NUM entries but arcmsr_alloc_ccb_pool()
only fills [0, maxFreeCCB), and shrinks maxFreeCCB further if the pool
has to stop early at a 4GB boundary, so everything from maxFreeCCB up is
a NULL pointer.  Comparing against the array size would let those
indices through to be dereferenced.

Also clamp maxFreeCCB to the array in arcmsr_get_firmware_spec().  It is
derived from the host_can_queue module parameter and gets 64 added to it
when it is below ARCMSR_MAX_FREECCB_NUM, so with can_queue in
[961, 1023] it ends up above the array it indexes -- which the pool loop
itself then walks past.  With the clamp in place, maxFreeCCB is both the
populated range and a valid bound.

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 slot inside the array but above maxFreeCCB, such as 200 with the
default maxFreeCCB of 192, loads a NULL entry and faults in the same
place:

  Oops: general protection fault, probably for non-canonical address
  0xdffffc0000000003: 0000 [#1] SMP KASAN NOPTI
  RIP: 0010:arcmsr_drain_donequeue+0x27/0x360 [arcmsr]

Both are refused with the check in place, a queue holding a rejected
entry followed by a good one drains to the producer index and the good
completion is reaped, and 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 | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/drivers/scsi/arcmsr/arcmsr_hba.c b/drivers/scsi/arcmsr/arcmsr_hba.c
index 255f1d898079f6..e635b76daae9a0 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 >= acb->maxFreeCCB) {
+			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 >= acb->maxFreeCCB) {
+			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;
@@ -3572,6 +3589,8 @@ static bool arcmsr_get_firmware_spec(struct AdapterControlBlock *acb)
 	acb->maxFreeCCB = acb->host->can_queue;
 	if (acb->maxFreeCCB < ARCMSR_MAX_FREECCB_NUM)
 		acb->maxFreeCCB += 64;
+	if (acb->maxFreeCCB > ARCMSR_MAX_FREECCB_NUM)
+		acb->maxFreeCCB = ARCMSR_MAX_FREECCB_NUM;
 	return rtn;
 }
 
@@ -3880,6 +3899,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;
+		}
 		pCCB = acb->pccb_pool[cmdSMID];
 		poll_ccb_done |= (pCCB == poll_ccb) ? 1 : 0;
 		/* check if command done with no error*/
-- 
2.43.0

[PATCH v4 3/4] 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 e635b76daae9a0..d13bd0febb81ac 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)
@@ -3671,7 +3671,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

[PATCH v4 4/4] scsi: arcmsr: serialise the type B completion ring

[Yehyeong Lee]

Type B keeps its completion ring in shared memory and consumes it from
arcmsr_hbaB_postqueue_isr(), from arcmsr_hbaB_polling_ccbdone() by way of
arcmsr_abort(), and from arcmsr_done4abort_postqueue().  None takes
acb->doneq_lock, and all three read reg->doneq_index and
reg->done_qbuffer[] with no other synchronisation, so two can fetch the
same entry.  arcmsr_abort() masks the outbound interrupts first, but that
only stops the next one; a handler already running on another CPU keeps
going and nothing waits for it.  The interrupt side clears the slot and
advances the index only after arcmsr_drain_donequeue() has run, so a
polling loop inside that window reads the same index and the same entry,
and the startdone test both consult is a plain read: both can reach
arcmsr_ccb_complete(), which means two scsi_dma_unmap() calls, two
scsi_done() calls and the command on ccb_free_list twice.  It needs no
misbehaving adapter, only a command timing out as it is completed.

A harness that plants one entry and runs the two consumers over it, on a
KASAN and lockdep kernel, has both take it in 5 of 5 runs and none in 5
runs with this patch.  I have no arcmsr hardware.

Take acb->doneq_lock in all three, as types D, E and F already do:
across the drain in the interrupt handler and in the reset sweep, and
across the index-and-entry step only in the polling loop, which must not
hold it while it completes a command.  Types A and C read completions
from a hardware queue port where each read pops a different entry, so
they are left alone.

Fixes: 1a4f550a09f8 ("[SCSI] arcmsr: 1.20.00.15: add SATA RAID plus other fixes")
Signed-off-by: Yehyeong Lee <yhlee@isslab.korea.ac.kr>
---
This touches the two lines patch 3 touches.

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

diff --git a/drivers/scsi/arcmsr/arcmsr_hba.c b/drivers/scsi/arcmsr/arcmsr_hba.c
index d13bd0febb81ac..9b2a7053f1823f 100644
--- a/drivers/scsi/arcmsr/arcmsr_hba.c
+++ b/drivers/scsi/arcmsr/arcmsr_hba.c
@@ -1552,8 +1552,11 @@ static void arcmsr_done4abort_postqueue(struct AdapterControlBlock *acb)
 
 	case ACB_ADAPTER_TYPE_B: {
 		struct MessageUnit_B *reg = acb->pmuB;
+		unsigned long flags;
+
 		/*clear all outbound posted Q*/
 		writel(ARCMSR_DOORBELL_INT_CLEAR_PATTERN, reg->iop2drv_doorbell); /* clear doorbell interrupt */
+		spin_lock_irqsave(&acb->doneq_lock, flags);
 		for (i = 0; i < ARCMSR_MAX_HBB_POSTQUEUE; i++) {
 			flag_ccb = reg->done_qbuffer[i];
 			if (flag_ccb != 0) {
@@ -1570,6 +1573,7 @@ static void arcmsr_done4abort_postqueue(struct AdapterControlBlock *acb)
 		}
 		reg->doneq_index = 0;
 		reg->postq_index = 0;
+		spin_unlock_irqrestore(&acb->doneq_lock, flags);
 		}
 		break;
 	case ACB_ADAPTER_TYPE_C: {
@@ -2566,8 +2570,9 @@ static void arcmsr_hbaB_postqueue_isr(struct AdapterControlBlock *acb)
 	struct ARCMSR_CDB *pARCMSR_CDB;
 	struct CommandControlBlock *pCCB;
 	bool error;
-	unsigned long cdb_phy_addr;
+	unsigned long cdb_phy_addr, flags;
 
+	spin_lock_irqsave(&acb->doneq_lock, flags);
 	index = reg->doneq_index % ARCMSR_MAX_HBB_POSTQUEUE;
 	while ((flag_ccb = reg->done_qbuffer[index]) != 0) {
 		cdb_phy_addr = (flag_ccb << 5) & 0xffffffff;
@@ -2582,6 +2587,7 @@ static void arcmsr_hbaB_postqueue_isr(struct AdapterControlBlock *acb)
 		index %= ARCMSR_MAX_HBB_POSTQUEUE;
 		reg->doneq_index = index;
 	}
+	spin_unlock_irqrestore(&acb->doneq_lock, flags);
 }
 
 static void arcmsr_hbaC_postqueue_isr(struct AdapterControlBlock *acb)
@@ -3664,16 +3670,18 @@ static int arcmsr_hbaB_polling_ccbdone(struct AdapterControlBlock *acb,
 	uint32_t flag_ccb, poll_ccb_done = 0, poll_count = 0;
 	int index, rtn;
 	bool error;
-	unsigned long ccb_cdb_phy;
+	unsigned long ccb_cdb_phy, flags;
 
 polling_hbb_ccb_retry:
 	poll_count++;
 	/* clear doorbell interrupt */
 	writel(ARCMSR_DOORBELL_INT_CLEAR_PATTERN, reg->iop2drv_doorbell);
 	while(1){
+		spin_lock_irqsave(&acb->doneq_lock, flags);
 		index = reg->doneq_index % ARCMSR_MAX_HBB_POSTQUEUE;
 		flag_ccb = reg->done_qbuffer[index];
 		if (flag_ccb == 0) {
+			spin_unlock_irqrestore(&acb->doneq_lock, flags);
 			if (poll_ccb_done){
 				rtn = SUCCESS;
 				break;
@@ -3691,6 +3699,7 @@ static int arcmsr_hbaB_polling_ccbdone(struct AdapterControlBlock *acb,
 		/*if last index number set it to 0 */
 		index %= ARCMSR_MAX_HBB_POSTQUEUE;
 		reg->doneq_index = index;
+		spin_unlock_irqrestore(&acb->doneq_lock, flags);
 		/* check if command done with no error*/
 		ccb_cdb_phy = (flag_ccb << 5) & 0xffffffff;
 		if (acb->cdb_phyadd_hipart)
-- 
2.43.0

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

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Low] scsi: arcmsr: Fixes trailer cites 1c57e86d75cf instead of
  1a4f550a09f8
--

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

> scsi: arcmsr: wrap the type B completion index before using it
>
> 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>

[Severity: Low]
1c57e86d75cf ("[SCSI] arcmsr: initial driver, version 1.20.00.13") only
supported Type A adapters.  Type B adapter support, struct MessageUnit_B,
and the unmasked reg->doneq_index reads in arcmsr_hbaB_postqueue_isr() and
arcmsr_hbaB_polling_ccbdone() (originally named arcmsr_hbb_postqueue_isr()
and arcmsr_polling_hbb_ccbdone()) were introduced later.  Should the Fixes:
tag be:

  Fixes: 1a4f550a09f8 ("[SCSI] arcmsr: 1.20.00.15: add SATA RAID plus other fixes")

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

Re: [PATCH v4 3/4] scsi: arcmsr: wrap the type B completion index

[Yehyeong Lee]

The review above is right about the Fixes tag on this patch.
1c57e86d75cf has no type B code at all; struct MessageUnit_B and both
doneq_index readers arrived with 1a4f550a09f8, so the tag should be:

Fixes: 1a4f550a09f8 ("[SCSI] arcmsr: 1.20.00.15: add SATA RAID plus other fixes")

Nothing else changes.  I can send a v5 with only that line corrected
right away, but I did not want to put another version in front of you
just for a tag.  Martin, would you prefer a v5, or could you fix the tag
up when applying?

Best regards,
Yehyeong Lee