[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