[PATCH 0/3] scsi: aic94xx: validate the sequencer-supplied EDB index

[PATCH 0/3] scsi: aic94xx: validate the sequencer-supplied EDB index

[Yehyeong Lee]

aic94xx derives an index into seq->edb_arr[] from a byte the sequencer
wrote, in three places, and subtracts one from it:

  aic94xx_scb.c  escb_tasklet_complete()
                   (dl->opcode & DL_PHY_MASK) - 1
  aic94xx_task.c asd_get_response_tasklet()
                   ((resp_sb->flags & 0x70) >> 4) - 1
  aic94xx_tmf.c  asd_get_tmf_resp_tasklet()      same

The field is three bits in each case, so the result runs from -1 to 6 while
the designed range is [0, ASD_EDBS_PER_SCB). escb_tasklet_complete()
already notices -- it has a check that prints "edb is 0x%x!" -- but the
block falls through, and the other two do not look at all.
asd_assign_edbs2escbs() gives the first ESCB edb_index 0, so for that one
an index of -1 reads seq->edb_arr[-1]; for the others it reads the last
buffer of the previous ESCB.

 1  add the missing return to the check escb_tasklet_complete() has.
    This also covers asd_invalidate_edb() at that function's out:
    label, which indexes escb->eb[] and seq->edb_arr[] alike.
 2  the same check in asd_get_response_tasklet(), which has none.
 3  the same check in asd_get_tmf_resp_tasklet(), which has none.

What I cannot tell you is whether the sequencer ever writes the value that
makes this -1. The sequencer is firmware loaded with request_firmware();
status_block[] is an opaque u8[4] in aic94xx_sas.h whose layout appears
only as a struct declared locally in aic94xx_task.c and copied into
aic94xx_tmf.c; there is no Documentation/ file and no MAINTAINERS entry for
this driver to ask. So these are not fixes for a demonstrated out-of-bounds
access, and I have not written them up as such. What they do is stop the
driver indexing an array with a value it either has already decided is
invalid, or never looked at. The driver clamps a device-supplied length in
asd_bytes_dmaed_tasklet() a few lines after indexing seq->edb_arr[] with
a device-supplied index and no bound, which is the inconsistency these
close.

Patch 1 carries Cc: stable, because completing a check the author wrote
stands on its own. Patches 2 and 3 add checks that were never there; I have
left stable off those and would rather a maintainer decide.

Worth naming: a response dropped by any of these leaves its ESCB
uninvalidated and so not reposted to the sequencer, because the repost
happens inside asd_invalidate_edb(). That costs one empty buffer per
malformed index. Recovering the ESCB instead would mean picking some other
buffer to invalidate, which the index does not tell us, so I did not invent
that.

I have not reproduced this on a controller; injecting edb_id = -1 through a
KUnit test on a KASAN kernel does reach seq->edb_arr[-1], which shows the
index arithmetic is wrong but not that the sequencer emits that value.

Yehyeong Lee (3):
  scsi: aic94xx: bail out of escb_tasklet_complete() on a bad EDB index
  scsi: aic94xx: validate the EDB index in asd_get_response_tasklet()
  scsi: aic94xx: validate the EDB index in asd_get_tmf_resp_tasklet()

 drivers/scsi/aic94xx/aic94xx_scb.c  | 1 +
 drivers/scsi/aic94xx/aic94xx_task.c | 6 ++++++
 drivers/scsi/aic94xx/aic94xx_tmf.c  | 6 ++++++
 3 files changed, 13 insertions(+)


base-commit: 2c3418fffa9d037b2038a6db48be63f9e2291806
-- 
2.43.0

[PATCH 1/3] scsi: aic94xx: bail out of escb_tasklet_complete() on a bad EDB index

[Yehyeong Lee]

escb_tasklet_complete() derives the empty data buffer index from the done
list opcode the sequencer wrote:

	int edb = (dl->opcode & DL_PHY_MASK) - 1; /* [0xc1,0xc7] -> [0,6] */

DL_PHY_MASK is 7, so the subtraction yields -1 through 6, and the
function already says that anything outside [0,6] is wrong:

	if (edb > 6 || edb < 0) {
		ASD_DPRINTK("edb is 0x%x! dl->opcode is 0x%x\n", ...);
		...
	}

The block reports the problem and then falls through to let it happen.
edb is used twice afterwards: asd_bytes_dmaed_tasklet() adds
ascb->edb_index to it to index seq->edb_arr[], and the out: label at the
end of the function hands the same value to asd_invalidate_edb(), which
indexes both escb->eb[] and seq->edb_arr[]. asd_assign_edbs2escbs() gives
the first ESCB edb_index 0, so for that one edb == -1 reads
seq->edb_arr[-1], one pointer before the array, and dereferences whatever
it finds.

Finish the check the way it reads: return instead of falling through.

Whether the sequencer can really report an opcode that makes this zero is
not something the tree answers. It is firmware loaded with
request_firmware(), the done list opcode is described only by the comment
above, and there is no Documentation/ file and no MAINTAINERS entry for
this driver. This is not a claim of a reachable out-of-bounds access,
only that the driver goes on to index an array with a value it has just
decided is invalid. The intent to bound what the sequencer supplies is
already in the same file, in asd_bytes_dmaed_tasklet():

	size = min(size, (u16) sizeof(phy->frame_rcvd));

which clamps a device-supplied length a few lines after indexing
seq->edb_arr[] with a device-supplied index and no bound.

One consequence worth naming: an ESCB dropped this way is not invalidated
and so is not reposted to the sequencer, because the repost happens
inside asd_invalidate_edb(). That takes one empty buffer out of
circulation per malformed opcode. It is the lesser of the two, and
recovering the ESCB instead would mean choosing some other buffer to
invalidate, which this index cannot tell us.

Fixes: 2908d778ab3e ("[SCSI] aic94xx: new driver")
Cc: stable@vger.kernel.org
Signed-off-by: Yehyeong Lee <yhlee@isslab.korea.ac.kr>
---
 drivers/scsi/aic94xx/aic94xx_scb.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/scsi/aic94xx/aic94xx_scb.c b/drivers/scsi/aic94xx/aic94xx_scb.c
index 08c8dad9ad621..d050e5325b943 100644
--- a/drivers/scsi/aic94xx/aic94xx_scb.c
+++ b/drivers/scsi/aic94xx/aic94xx_scb.c
@@ -421,6 +421,7 @@ static void escb_tasklet_complete(struct asd_ascb *ascb,
 			    le64_to_cpu(ascb->scb->header.next_scb),
 			    le16_to_cpu(ascb->scb->header.index),
 			    ascb->scb->header.opcode);
+		return;
 	}
 
 	/* Catch these before we mask off the sb_opcode bits */
-- 
2.43.0

[PATCH 2/3] scsi: aic94xx: validate the EDB index in asd_get_response_tasklet()

[Yehyeong Lee]

asd_get_response_tasklet() takes the empty data buffer index out of the
done list status block the sequencer wrote, with the same arithmetic
escb_tasklet_complete() uses on its own copy:

	int  edb_id = ((resp_sb->flags & 0x70) >> 4)-1;

The extracted field is three bits, so the subtraction yields -1 through
6, and the function indexes seq->edb_arr[] with it without looking:

	edb = asd_ha->seq.edb_arr[edb_id + escb->edb_index];
	r = edb->vaddr;

asd_assign_edbs2escbs() gives the first ESCB edb_index 0, so for that one
edb_id == -1 reads seq->edb_arr[-1], one pointer before the kmalloc'd
array, and the next line dereferences it. asd_invalidate_edb() at the end
of the function would index escb->eb[] and seq->edb_arr[] with the same
value.

Reject an index outside [0, ASD_EDBS_PER_SCB) and return, the way the
lookup failure just above it already returns. That check stays as it is;
this is about the other value taken from the same status block.

The preceding patch added the missing return to the check
escb_tasklet_complete() already had. This is that check in a function
that never had one. It is written against ASD_EDBS_PER_SCB rather than
the literal 6 that function uses, because the constant is what sizes
escb->eb[] and, through asd_assign_edbs2escbs(), the stride within
seq->edb_arr[]; the existing condition is left alone.

Whether the sequencer can really report zero in that field is not
something the tree answers. It is firmware loaded with
request_firmware(), status_block[] is an opaque four bytes in
aic94xx_sas.h whose layout is described only by a struct declared locally
in this function and again in aic94xx_tmf.c, and there is no
Documentation/ file or MAINTAINERS entry for this driver. This is not a
claim of a reachable out-of-bounds access. It makes the driver consistent
with itself: asd_bytes_dmaed_tasklet() in aic94xx_scb.c clamps a
device-supplied length,

	size = min(size, (u16) sizeof(phy->frame_rcvd));

a few lines after indexing seq->edb_arr[] with a device-supplied
index and no bound of its own.

As with the preceding patch, a response dropped here leaves its ESCB
uninvalidated and so not reposted, since the repost happens inside
asd_invalidate_edb(). The alternative is to index the array with a value
the driver cannot place.

Fixes: 2908d778ab3e ("[SCSI] aic94xx: new driver")
Signed-off-by: Yehyeong Lee <yhlee@isslab.korea.ac.kr>
---
 drivers/scsi/aic94xx/aic94xx_task.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/scsi/aic94xx/aic94xx_task.c b/drivers/scsi/aic94xx/aic94xx_task.c
index b26a468ddc981..35cf61f541d1b 100644
--- a/drivers/scsi/aic94xx/aic94xx_task.c
+++ b/drivers/scsi/aic94xx/aic94xx_task.c
@@ -169,6 +169,12 @@ static void asd_get_response_tasklet(struct asd_ascb *ascb,
 		return;
 	}
 
+	if (edb_id < 0 || edb_id >= ASD_EDBS_PER_SCB) {
+		ASD_DPRINTK("edb_id is %d! resp_sb->flags is 0x%x\n",
+			    edb_id, resp_sb->flags);
+		return;
+	}
+
 	ts->buf_valid_size = 0;
 	edb = asd_ha->seq.edb_arr[edb_id + escb->edb_index];
 	r = edb->vaddr;
-- 
2.43.0

[PATCH 3/3] scsi: aic94xx: validate the EDB index in asd_get_tmf_resp_tasklet()

[Yehyeong Lee]

asd_get_tmf_resp_tasklet() extracts the empty data buffer index from the
done list status block exactly as asd_get_response_tasklet() does:

	int  edb_id = ((resp_sb->flags & 0x70) >> 4)-1;

and indexes seq->edb_arr[] with it unchecked:

	edb = asd_ha->seq.edb_arr[edb_id + escb->edb_index];
	ascb->tag = *(__be16 *)(edb->vaddr+4);

The field is three bits, so the value runs from -1 to 6, and the first
ESCB has edb_index 0, which makes seq->edb_arr[-1] reachable arithmetic.
asd_invalidate_edb() at the end of the function uses the same value
again.

Apply the same check as the preceding patch and return res, which is
TMF_RESP_FUNC_FAILED at that point, the way the lookup failure above it
already does.

The caveats are the ones on that patch: the sequencer is firmware loaded
with request_firmware() and nothing in the tree describes what it may put
in this field, so this is not a claim of a reachable out-of-bounds
access, and a response dropped here leaves its ESCB uninvalidated and
unreposted.

Fixes: 2908d778ab3e ("[SCSI] aic94xx: new driver")
Signed-off-by: Yehyeong Lee <yhlee@isslab.korea.ac.kr>
---
 drivers/scsi/aic94xx/aic94xx_tmf.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/scsi/aic94xx/aic94xx_tmf.c b/drivers/scsi/aic94xx/aic94xx_tmf.c
index 28ac92b041fe3..1ee6009d83051 100644
--- a/drivers/scsi/aic94xx/aic94xx_tmf.c
+++ b/drivers/scsi/aic94xx/aic94xx_tmf.c
@@ -282,6 +282,12 @@ static int asd_get_tmf_resp_tasklet(struct asd_ascb *ascb,
 		return res;
 	}
 
+	if (edb_id < 0 || edb_id >= ASD_EDBS_PER_SCB) {
+		ASD_DPRINTK("edb_id is %d! resp_sb->flags is 0x%x\n",
+			    edb_id, resp_sb->flags);
+		return res;
+	}
+
 	edb = asd_ha->seq.edb_arr[edb_id + escb->edb_index];
 	ascb->tag = *(__be16 *)(edb->vaddr+4);
 	fh = edb->vaddr + 16;
-- 
2.43.0

Re: [PATCH 2/3] scsi: aic94xx: validate the EDB index in asd_get_response_tasklet()

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] scsi: aic94xx: dropped task response leads to false-positive SUCCESS
--

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

scsi: aic94xx: validate the EDB index in asd_get_response_tasklet()

This commit validates the empty data buffer index in
asd_get_response_tasklet() to prevent out-of-bounds array access. If the
sequencer returns an invalid edb_id, the driver now prints a debug message
and returns early.

> diff --git a/drivers/scsi/aic94xx/aic94xx_task.c b/drivers/scsi/aic94xx/aic94xx_task.c
> index b26a468ddc981..35cf61f541d1b 100644
> --- a/drivers/scsi/aic94xx/aic94xx_task.c
> +++ b/drivers/scsi/aic94xx/aic94xx_task.c
> @@ -169,6 +169,12 @@ static void asd_get_response_tasklet(struct asd_ascb *ascb,
>  		return;
>  	}
>  
> +	if (edb_id < 0 || edb_id >= ASD_EDBS_PER_SCB) {
> +		ASD_DPRINTK("edb_id is %d! resp_sb->flags is 0x%x\n",
> +			    edb_id, resp_sb->flags);
> +		return;
> +	}
        ^^^^^^^

[Severity: Medium]
Does returning early here leave the task status as SAS_PROTO_RESPONSE?

If this return is taken, ts->stat is not updated to an error status. For SSP
tasks, sas_end_task() in libsas maps SAS_PROTO_RESPONSE to DID_OK and
SAS_SAM_STAT_GOOD. For ATA tasks, libsas parses the task's uninitialized
response buffer, resulting in no error mask.

Could this lead to a false-positive task success, masking hardware errors or
causing silent data corruption if data was not actually transferred?

> +
>  	ts->buf_valid_size = 0;
>  	edb = asd_ha->seq.edb_arr[edb_id + escb->edb_index];
>  	r = edb->vaddr;

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

Re: [PATCH 3/3] scsi: aic94xx: validate the EDB index in asd_get_tmf_resp_tasklet()

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] scsi: aic94xx: type confusion and OOB array access due to unchecked index_escb
--

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

scsi: aic94xx: validate the EDB index in asd_get_tmf_resp_tasklet()

This patch validates the empty data buffer index (edb_id) extracted from the
done list status block in asd_get_tmf_resp_tasklet(). It adds bounds checking
before indexing seq->edb_arr[] to prevent out-of-bounds access.

> diff --git a/drivers/scsi/aic94xx/aic94xx_tmf.c b/drivers/scsi/aic94xx/aic94xx_tmf.c
> index 28ac92b041fe3..1ee6009d83051 100644
> --- a/drivers/scsi/aic94xx/aic94xx_tmf.c
> +++ b/drivers/scsi/aic94xx/aic94xx_tmf.c
> @@ -282,6 +282,12 @@ static int asd_get_tmf_resp_tasklet(struct asd_ascb *ascb,
>  		return res;
>  	}
>  
> +	if (edb_id < 0 || edb_id >= ASD_EDBS_PER_SCB) {
> +		ASD_DPRINTK("edb_id is %d! resp_sb->flags is 0x%x\n",
> +			    edb_id, resp_sb->flags);

[Severity: High]
Does resp_sb->index_escb need validation as well?

Looking at asd_get_tmf_resp_tasklet(), the index_escb value is extracted and
passed directly to asd_tc_index_find():

drivers/scsi/aic94xx/aic94xx_tmf.c:asd_get_tmf_resp_tasklet() {
    ...
	escb = asd_tc_index_find(&asd_ha->seq,
				 (int)le16_to_cpu(resp_sb->index_escb));
    ...
}

Since index_escb comes from the untrusted firmware status block, could
an invalid index cause an out-of-bounds read in asd_tc_index_find() across
seq->tc_index_array?

Furthermore, could this lead to type confusion? The returned escb pointer
is used at the end of the function:

drivers/scsi/aic94xx/aic94xx_tmf.c:asd_get_tmf_resp_tasklet() {
    ...
	asd_invalidate_edb(escb, edb_id);
	return res;
}

If a normal SCB index was returned instead of an Empty SCB, could
asd_invalidate_edb() corrupt the state of normal pending tasks by assuming
the pointer points to an Empty SCB without verifying its opcode?

> +		return res;
> +	}
> +
>  	edb = asd_ha->seq.edb_arr[edb_id + escb->edb_index];
>  	ascb->tag = *(__be16 *)(edb->vaddr+4);
>  	fh = edb->vaddr + 16;

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