[PATCH v8 0/8] scsi: mvumi: bound the values the firmware supplies

[Yehyeong Lee]

This supersedes

  [PATCH v7 0/7] scsi: mvumi: bound the values the firmware supplies
  <20261015005717.3719956-1-yhlee@xxxxxxxxxxxxxxxxxx>

which can be dropped.

Patches 1-7 are unchanged from v7; patch 8 is new.

Everything the firmware writes reaches the driver either through the
handshake page, through the event buffer, or through a response frame in
the outbound list.  The patches follow that split, with one at the end that
is unrelated:

 1  event buffer: param_count and sense_data_length index fixed arrays in
    mvumi_show_event().
 2  handshake page: frame_length drives the checksum walk, whose counter is
    an unsigned char, so 256 or more never terminates.
 3  event buffer: the hotplug bitmap size drives two walks with nothing
    holding it inside what the firmware was asked for.
 4  event buffer: those walks cast a four-byte-aligned bitmap to
    unsigned long *.
 5  handshake page: the entry sizes and max_io size three allocations, the
    list depth drives the ring index, the device count has to fit the byte
    mvumi_build_frame() gives it, a reset reports all of them again without
    reallocating anything, and every field is read once.
 6  response frame: the tag indexes tag_cmd[], and the value that is
    checked has to be the value that is used.
 7  DMA teardown: the address mvumi_delete_internal_cmd() rebuilds from the
    SGL loses its upper 32 bits.
 8  outbound list: the 0xff the driver stamps over the tag of an entry it
    has consumed is a tag in range once the firmware asks for more than 255
    commands, so a stale entry is no longer turned away.  Stamp 0xffff,
    which is outside the u16 tag range whatever the firmware asks for.

Patch 8 is what the review of v7 6/7 pointed at.  I answered that review
saying I would send it as a follow-up once the series landed; it is small
and self-contained, so it is here instead.

Three other points from that review I am not changing, and why.

Endianness: the driver does no device-to-host endian conversion anywhere,
and these fields are plain u16 used natively throughout (host->max_id =
mhba->max_target_id, tag_pool.size = mhba->max_io).  The new checks read
the same values the driver already consumes, so they follow the existing
convention rather than change it.  Making the driver big-endian-correct
would be a driver-wide __le conversion, which is a separate change.  On a
big-endian build the base driver silently accepts a byte-swapped device
count and scans phantom targets; patch 5 turns that into an explicit probe
failure, so the behaviour there does change.

MV9580 and the per-completion udelay: the unconditional request_id
comparison in mvumi_receive_ob_list_entry() is pre-existing.  Patch 6 wraps
the reads in READ_ONCE(), corrects the > to >=, and localizes the tag; the
comparison itself is unchanged.  The re-check patch 6 adds in the
completion handler is gated by request_id_enabled, matching the gate
already in mvumi_check_ob_frame(), so a controller that leaves the flag
clear is unaffected.

dma_rmb(): the validate-then-memcpy ordering is unchanged from the base.
The ownership handshake is the MMIO copy pointer read with ioread32() in
mvumi_check_ob_list_9143()/_9580(), which already carries the
architecture's read barrier, not the tag.  The driver's own comment says an
entry can be visible "before entry has valid data", so the tag can itself
be stale; ordering the tag read ahead of the payload read would not help,
and a barrier cannot make a payload that has not been written yet arrive.

One more thing I left alone: the second hotplug bitmap starts at
bitmap[size >> 3], which is only self-consistent when size is a multiple of
eight, and nothing in the tree says whether the firmware packs the two
bitmaps bit by bit or aligns the second to a byte.  Either reading makes
the index wrong for a size that is not a multiple of eight, but correcting
it one way breaks the other, so patch 4 keeps the existing numbering
exactly.

I don't have mvumi hardware; these are reasoned from the code, not
reproduced on a controller.

Yehyeong Lee (8):
  scsi: mvumi: bound the per-event parameter and sense dumps
  scsi: mvumi: reject an out-of-range handshake frame length
  scsi: mvumi: bound the hotplug event bitmap
  scsi: mvumi: walk the hotplug event bitmap a byte at a time
  scsi: mvumi: reject handshake sizes that overflow the list allocations
  scsi: mvumi: validate the response frame tag before using it
  scsi: mvumi: fix the DMA address rebuilt in
    mvumi_delete_internal_cmd()
  scsi: mvumi: stamp a consumed outbound list entry out of range

 drivers/scsi/mvumi.c | 240 ++++++++++++++++++++++++++++++++++---------
 drivers/scsi/mvumi.h |  15 +++
 2 files changed, 204 insertions(+), 51 deletions(-)


base-commit: 24a19b953e2a3d0ea5a8c4951a0ab8f32720c907
-- 
2.43.0