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

[PATCH 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.

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 | 23 +++++++++++++++++++++--
 1 file changed, 21 insertions(+), 2 deletions(-)

-- 
2.43.0

[PATCH 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

[PATCH 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 of which only acb->maxFreeCCB are
ever filled in.  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.

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.

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 | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

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;
+		}
 		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;
+		}
 		ccb = acb->pccb_pool[cmdSMID];
 		error = (acb->pCompletionQ[doneq_index].cmdFlag &
 			ARCMSR_CCBREPLY_FLAG_ERROR_MODE1) ? true : false;
@@ -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;
+		}
 		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 2/3] scsi: arcmsr: check the completion index the adapter reports

[sashiko-bot]

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

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

[Yehyeong Lee]

Thanks for the review.

On the interrupt storm: you are right.  Breaking out of the loop left
the completion consumer index unadvanced, so the adapter would keep the
postqueue interrupt asserted and the handler would re-enter on the same
entry.  v2 skips the bad entry and continues so the consumer index
advances past it, clearing it to 0xffff on type F as the normal path
does; the queue drains instead of stalling.

On maxFreeCCB: also correct.  arcmsr_get_firmware_spec() can raise
acb->maxFreeCCB above the 1024-entry pccb_pool, so v2 bounds the index
against ARCMSR_MAX_FREECCB_NUM instead.

On the uncleared cmdSMID in arcmsr_hbaE_polling_ccbdone(): this predates
the change -- the polling path never writes 0xffff back for type F even
on the normal consume path -- so it is independent of the index bound
and I will address it separately rather than fold it into this fix.

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

[PATCH 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 1381420d9fbd7..b73aaa0c8068c 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)
@@ -3662,7 +3662,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