[PATCH v3 0/5] scsi: aic94xx: validate the indices the sequencer supplies

[PATCH v3 0/5] scsi: aic94xx: validate the indices the sequencer supplies

[Yehyeong Lee]

This supersedes

  [PATCH v2 0/5] scsi: aic94xx: validate the indices the sequencer supplies
  <20261015081935.4168771-1-yhlee@isslab.korea.ac.kr>

which should be dropped.  Patches 1 to 4 are the same commits; only patch 5
changed.

Review of v2 5/5 asked whether reading escb->scb->header.opcode outside
tc_index_lock races with a concurrent free of the aSCB the index landed on.
It does, so v3 moves the check inside the critical section the lookup
already takes, and clears escb on a mismatch so the existing "no escb for
this dl" bail handles it.  That is sound for a specific reason:
asd_ascb_free() clears the array slot under tc_index_lock and only then
frees ascb->scb and the aSCB, so a slot seen non-NULL while the lock is
held belongs to an aSCB whose free has not started.  It also makes the
patch smaller -- two lines per file instead of a separate bail block in
each, because the NULL path was already there.

Worth saying, since the review framed it as introduced by the patch: the
driver already uses that pointer after the unlock.  Base reads
escb->edb_index and calls asd_invalidate_edb(), which dereferences
escb->scb and writes eb->flags and escb->num_valid, all outside the lock.
v2's opcode read sat in the same window, and in the case it is about it
bailed before those, so it narrowed the lockless use rather than widening
it.  That older and wider window is still there after this series; closing
it means either holding the lock across asd_invalidate_edb(), which reaches
MMIO through asd_post_escb_list(), or giving aSCBs a refcount.  Neither
belongs in this series, and I did not want to bundle it silently.

The series as a whole: aic94xx takes two numbers out of the done list the
sequencer writes and uses both without looking at them.

 1  escb_tasklet_complete() already noticed a bad EDB index and printed
    about it, then fell through and used it anyway.  Add the missing
    return.
 2  asd_get_response_tasklet() never looked.  Bound it, and report
    SAS_INTERRUPTED on the way out rather than leaving the command looking
    successful.
 3  asd_get_tmf_resp_tasklet() never looked either.
 4  asd_tc_index_find() indexes tc_index_array[] with the number the
    sequencer echoes back, in all three of its callers, without a bound.
 5  the pointer that comes back is used as an Empty SCB without checking
    that it is one, and struct scb is a union.

I have not reproduced any of this on a controller.  Injecting the bad
values through a KUnit test on a KASAN kernel does reach seq->edb_arr[-1]
and tc_index_array[] past its end, and does leave a non-Empty SCB
modified, which shows the arithmetic and the missing checks are real; it
does not show that the sequencer emits those values, and nothing in the
tree answers that, since it is firmware loaded with request_firmware() and
there is no Documentation/ file or MAINTAINERS entry for this driver.  The
commit messages are written on that basis and claim no reachable memory
error.

Patches 1, 4 and 5 carry Cc: stable.  Patches 2 and 3 do not, and I would
rather a maintainer decide.

Yehyeong Lee (5):
  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()
  scsi: aic94xx: bound the transaction context index from the sequencer
  scsi: aic94xx: check that an ESCB lookup returned an ESCB

 drivers/scsi/aic94xx/aic94xx_hwi.h  |  2 ++
 drivers/scsi/aic94xx/aic94xx_scb.c  |  1 +
 drivers/scsi/aic94xx/aic94xx_task.c | 10 ++++++++++
 drivers/scsi/aic94xx/aic94xx_tmf.c  |  8 ++++++++
 4 files changed, 21 insertions(+)


base-commit: 2c3418fffa9d037b2038a6db48be63f9e2291806
-- 
2.43.0

[PATCH v3 1/5] 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 v3 2/5] 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, and set
ts->stat on the way out, because returning without touching it reports
the command as a success.

asd_task_tasklet_complete() sets ts->resp and ts->stat before it calls
this function:

	case TC_SSP_RESP:
	case TC_ATA_RESP:
		ts->resp = SAS_TASK_COMPLETE;
		ts->stat = SAS_PROTO_RESPONSE;
		asd_get_response_tasklet(ascb, dl);

SAS_PROTO_RESPONSE is a promise that this function will go on to put the
real status there, from the response IU for SSP or the ending FIS for
STP. Returning early breaks that promise and leaves the promise in place.
For SSP, sas_end_task() starts from DID_OK and SAS_SAM_STAT_GOOD, and its
SAS_PROTO_RESPONSE case only prints a notice, so the command completes as
a success. For STP, sas_ata_task_done() takes SAS_PROTO_RESPONSE as "the
ending FIS has the status" and copies ATA_RESP_FIS_SIZE out of ts->buf,
which this function never filled, then derives err_mask from it.

So the bail reports SAS_INTERRUPTED, which is what this driver already
uses for a response that arrived but could not be used -- TI_BREAK,
TI_PROTO_ERR, TI_NAK, TI_ACK_NAK_TO, TF_SMP_XMIT_RCV_ERR and
TC_ATA_R_ERR_RECV all map to it. sas_end_task() turns it into
DID_NO_CONNECT, and sas_to_ata_err() into AC_ERR_ATA_BUS, so neither path
mistakes it for a completed command.

The lookup failure above it had the same problem and gets the same line.
Nothing changes on the path where the index is in range.

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

diff --git a/drivers/scsi/aic94xx/aic94xx_task.c b/drivers/scsi/aic94xx/aic94xx_task.c
index b26a468ddc981..bb5094ca5de15 100644
--- a/drivers/scsi/aic94xx/aic94xx_task.c
+++ b/drivers/scsi/aic94xx/aic94xx_task.c
@@ -166,6 +166,14 @@ static void asd_get_response_tasklet(struct asd_ascb *ascb,
 
 	if (!escb) {
 		ASD_DPRINTK("Uh-oh! No escb for this dl?!\n");
+		ts->stat = SAS_INTERRUPTED;
+		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);
+		ts->stat = SAS_INTERRUPTED;
 		return;
 	}
 
-- 
2.43.0

[PATCH v3 3/5] 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

[PATCH v3 4/5] scsi: aic94xx: bound the transaction context index from the sequencer

[Yehyeong Lee]

Every aSCB the driver allocates takes a slot in one shared index array,
and the driver tells the sequencer which slot it used:

	/* asd_ascb_alloc() */
	ascb->tc_index = asd_tc_index_get(seq, ascb);
	...
	ascb->scb->header.index = cpu_to_le16((u16)ascb->tc_index);

The sequencer hands that number back in the done list, and the driver
looks the aSCB up again with it in three places: asd_dl_tasklet_handler()
uses dl->index, and asd_get_response_tasklet() and
asd_get_tmf_resp_tasklet() use the index_escb field of the status block.
All three go through

	static inline void *asd_tc_index_find(struct asd_seq_data *seq, int index)
	{
		return seq->tc_index_array[index];
	}

which does not look at index at all.

The array is kcalloc()ed with tc_index_bitmap_bits entries, which is
hw_prof.max_scbs, which is asd_get_cmdctx_size() divided by sizeof(struct
scb). The two values asd_get_cmdctx_size() can return are 32768 and
65536, and struct scb starts with an 11-byte scb_header, so max_scbs
cannot exceed 5957. Both numbers the sequencer supplies are __le16, so
either one can name a slot past the end of the array, and the pointer
read from there is then dereferenced.

Check the index where it is used. The three callers already handle a NULL
return -- "Uh-oh! No escb for this dl?!" in the two status block paths
and "BUG:sequencer:dl:no ascb?!" in the done list handler -- so none of
them needs to change.

This is not a claim of a reachable out-of-bounds access: the index
originates with the driver and comes back echoed, and whether the
sequencer can echo something else is not something the tree answers,
since it is firmware loaded with request_firmware() and there is no
Documentation/ file or MAINTAINERS entry for this driver. It is a bound
on a number that arrives from outside, in the one place all three users
share.

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_hwi.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/scsi/aic94xx/aic94xx_hwi.h b/drivers/scsi/aic94xx/aic94xx_hwi.h
index 930e192b1cd41..841769a8130a9 100644
--- a/drivers/scsi/aic94xx/aic94xx_hwi.h
+++ b/drivers/scsi/aic94xx/aic94xx_hwi.h
@@ -306,6 +306,8 @@ static inline int asd_tc_index_get(struct asd_seq_data *seq, void *ptr)
  */
 static inline void *asd_tc_index_find(struct asd_seq_data *seq, int index)
 {
+	if (index < 0 || index >= seq->tc_index_bitmap_bits)
+		return NULL;
 	return seq->tc_index_array[index];
 }
 
-- 
2.43.0

[PATCH v3 5/5] scsi: aic94xx: check that an ESCB lookup returned an ESCB

[Yehyeong Lee]

asd_get_response_tasklet() and asd_get_tmf_resp_tasklet() look an Empty
SCB up by the index_escb field of the done list status block and then use
it as one:

	escb = asd_tc_index_find(&asd_ha->seq,
				 (int)le16_to_cpu(resp_sb->index_escb));
	...
	edb = asd_ha->seq.edb_arr[edb_id + escb->edb_index];
	...
	asd_invalidate_edb(escb, edb_id);

Nothing checks that what came back is an Empty SCB. There is one index
space for every aSCB the driver allocates -- asd_ascb_alloc() is the only
caller of asd_tc_index_get() -- so an in-range index can just as well
name a live task, a control phy SCB or an internal command.

That matters because struct scb is a union, and asd_invalidate_edb()
reads its escb member without looking at the opcode:

	struct empty_scb *escb = &ascb->scb->escb;
	struct sg_el     *eb   = &escb->eb[edb_id];
	...
	eb->flags |= ELEMENT_NOT_VALID;
	escb->num_valid--;

On an SCB that is not an Empty SCB those two stores land in whichever
union member it really is -- for an initiate_ssp_task that region holds
the SSP frame header and the scatter-gather elements of a command that is
still in flight.

Check the opcode, the way asd_dl_tasklet_handler() already does when it
decides whether a done list entry belongs to an Empty SCB:

	} else if (ascb->scb->header.opcode == EMPTY_SCB) {

Do it inside the critical section that already holds tc_index_lock for
the lookup, and clear escb on a mismatch so that the existing "no escb
for this dl" path handles it. Reading the opcode after the unlock would
be reading an aSCB the lookup no longer pins: asd_ascb_free() clears the
array slot under tc_index_lock and only then calls dma_pool_free() on
ascb->dma_scb.vaddr, which is ascb->scb, and kmem_cache_free() on the
aSCB itself. So a slot seen non-NULL while the lock is held belongs to an
aSCB whose free has not reached its own critical section yet, and the
opcode is safe to read; the same read after the unlock is not.
asd_tc_index_find() is documented as needing that lock anyway.

Empty SCBs themselves are not freed while the adapter is up --
asd_free_escbs() runs from the teardown path -- so this only concerns an
index that names something other than an ESCB, which is the case the
check is for.

Both callers already bail on a NULL lookup, so neither needs a new exit:
asd_get_response_tasklet() reports SAS_INTERRUPTED there as of the
earlier patch in this series, and asd_get_tmf_resp_tasklet() returns the
TMF_RESP_FUNC_FAILED that res already holds. The non-ESCB case shares
that diagnostic rather than adding one.

This is not a claim of a reachable type confusion. The index originates
with the driver and the sequencer echoes it back, and whether it can echo
a different one is not something the tree answers. The check is cheap and
the idiom is already in the driver.

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_task.c | 2 ++
 drivers/scsi/aic94xx/aic94xx_tmf.c  | 2 ++
 2 files changed, 4 insertions(+)

diff --git a/drivers/scsi/aic94xx/aic94xx_task.c b/drivers/scsi/aic94xx/aic94xx_task.c
index bb5094ca5de15..00addeef1230a 100644
--- a/drivers/scsi/aic94xx/aic94xx_task.c
+++ b/drivers/scsi/aic94xx/aic94xx_task.c
@@ -162,6 +162,8 @@ static void asd_get_response_tasklet(struct asd_ascb *ascb,
 	spin_lock_irqsave(&asd_ha->seq.tc_index_lock, flags);
 	escb = asd_tc_index_find(&asd_ha->seq,
 				 (int)le16_to_cpu(resp_sb->index_escb));
+	if (escb && escb->scb->header.opcode != EMPTY_SCB)
+		escb = NULL;	  /* the index landed on something else */
 	spin_unlock_irqrestore(&asd_ha->seq.tc_index_lock, flags);
 
 	if (!escb) {
diff --git a/drivers/scsi/aic94xx/aic94xx_tmf.c b/drivers/scsi/aic94xx/aic94xx_tmf.c
index 1ee6009d83051..c05446d66b7bb 100644
--- a/drivers/scsi/aic94xx/aic94xx_tmf.c
+++ b/drivers/scsi/aic94xx/aic94xx_tmf.c
@@ -275,6 +275,8 @@ static int asd_get_tmf_resp_tasklet(struct asd_ascb *ascb,
 	spin_lock_irqsave(&asd_ha->seq.tc_index_lock, flags);
 	escb = asd_tc_index_find(&asd_ha->seq,
 				 (int)le16_to_cpu(resp_sb->index_escb));
+	if (escb && escb->scb->header.opcode != EMPTY_SCB)
+		escb = NULL;	  /* the index landed on something else */
 	spin_unlock_irqrestore(&asd_ha->seq.tc_index_lock, flags);
 
 	if (!escb) {
-- 
2.43.0

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

[James Bottomley]

First a meta question: the aic94xx is about 20 years old today ... have
you actually tested this on real hardware?

On Thu, 2026-10-15 at 18:17 +0900, Yehyeong Lee wrote:
> 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:

These are the sequencer response flags ... is there any evidence of the
sequencer ever setting these three bits to a zero value?

[...]
> +	if (edb_id < 0 || edb_id >= ASD_EDBS_PER_SCB) {

Why the second comparison?  As you say the value could only be -1..6
and ASD_EDBS_PER_SCB is defined to 7.

Regards,

James

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

[Yehyeong Lee]

> have you actually tested this on real hardware?

No -- the hardware is twenty years old and I couldn't get a board. I
exercised the path by fault injection: built the function into a KUnit
harness, fed it a status block with those three bits zero, and KASAN
reported the read one entry before seq->edb_arr[] without the guard and
nothing with it. That isn't evidence the sequencer does this, and I'm not
presenting it as such.

> is there any evidence of the sequencer ever setting these three bits
> to a zero value?

I can't show that it does. I looked for something authoritative -- an
AIC-9410 done-list/status-block layout -- and there's no public Adaptec
spec; the sequencer is an opaque request_firmware() blob, and the only
in-tree description of this field is the commented-out size line just
above the decode. What the tree does settle is the decode: the index is
one-based, and the same seq->edb_arr[edb_id + edb_index] access is also
reached from the empty-buffer path (escb_tasklet_complete ->
asd_bytes_dmaed_tasklet), where edb < 0 is rejected before the access --
aic94xx_scb.c:410, the 0xc0 "no buffer" case. The response path reaches
the identical access with no such check. So it really turns on whether
the sequencer can set those bits to zero on a response: if you know it
can't, this is dead code like the second comparison and I'll drop it; if
the field is as unbounded as it looks, this is the guard the driver
already applies to the same array on its other producer.

> Why the second comparison?  As you say the value could only be -1..6
> and ASD_EDBS_PER_SCB is defined to 7.

You're right, it's dead -- (resp_sb->flags & 0x70) >> 4 is 0..7, so
edb_id is -1..6 and -1 is the only out-of-range value. I'll cut it to

	if (edb_id < 0)

here and in the aic94xx_tmf.c copy. (scb.c carries both halves; I can
make them match whichever you prefer.)