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

This supersedes

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

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