[PATCH v9 0/9] scsi: mvumi: bound the values the firmware supplies

[Yehyeong Lee]

This supersedes

  [PATCH v8 0/8] scsi: mvumi: bound the values the firmware supplies
  <20261015022728.3936714-1-yhlee@xxxxxxxxxxxxxxxxxx>

which can be dropped.

Patches 1-6 and 8 are unchanged from v8.  Patch 7 replaces v8's 7/8.
Patch 9 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:

 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: mvumi_delete_internal_cmd() rebuilt the buffer address
    out of the command frame with a shift that cannot produce the upper
    half of it.  Rather than widen the shift, keep the address and the
    length in struct mvumi_cmd and stop reading the frame back.
 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.
 9  handshake: nothing bounds how many times mvumi_check_handshake() will
    answer the firmware, so a controller that keeps the handshake going
    keeps it going forever.  Count the steps and give up after 1024.

Patch 7 is what the review of v8 7/8 pointed at.  I had sent that as a
one-line fix to widen the shift; the review asked why the driver reads
those fields back out of the command frame at all, which is the better
question, so the read-back is gone instead and the shift with it.  The
truncation is not reachable as a bug: dma_set_mask() moves only the
streaming mask and nothing in this driver raises the coherent mask above
the 32 bits pci_device_add() sets, so dma_alloc_coherent() never returns
an address whose upper half could be lost.  No Cc: stable on that patch
for the same reason.

Patch 9 answers the review of v8 5/8, which said a rejected handshake page
would leave the driver retrying forever.  The quoted reason was a
"before = jiffies" reset inside the DRBL_HANDSHAKE_ISR branch, and there
is no such code in the tree -- mvumi_handshake_event() samples that once
on entry and times out after FW_MAX_DELAY -- but the outer loop in
mvumi_check_handshake() really is unbounded, so that is what patch 9
bounds.  It does not touch the doorbell writes or the HS_S_ABORT store, so
the FW_STATE_STARTING restart still works.

Two points from the v8 reviews I am not changing.

Endianness: the driver does no device-to-host endian conversion anywhere,
and these fields are plain u16 and u32 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 is a driver-wide change: fifteen multi-byte fields are
read from firmware-written memory without a conversion, across the
handshake header and page 1, the response frame, the event and hotplug
records, and the inbound frame fields the driver reads back.  I will send
that as its own series.  Patch 7 takes the SGL fields off that list, since
nothing reads them any more.  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.

dma_rmb(): both reads of mhba->ob_list, and the memcpy of the payload, are
reached only through mvumi_receive_ob_list_entry(), which begins by
calling mvumi_check_ob_list_9143() or _9580().  Both of those read the
MMIO copy pointer with ioread32() before returning, and that read carries
the architecture's read barrier, which orders every load after it, so a
dma_rmb() further down would be covered by a barrier that has already run.
Making the tag the thing that is ordered against instead would need the
controller to write the payload before the tag, and the tag is at offset 2
of the response frame while the payload starts at offset 8.  Nothing in
the tree states a write order, so I have left it.

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 (9):
  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: free the internal command buffer by what was allocated
  scsi: mvumi: stamp a consumed outbound list entry out of range
  scsi: mvumi: bound the firmware handshake retry loop

 drivers/scsi/mvumi.c | 264 ++++++++++++++++++++++++++++++++-----------
 drivers/scsi/mvumi.h |  36 ++++--
 2 files changed, 226 insertions(+), 74 deletions(-)


base-commit: 24a19b953e2a3d0ea5a8c4951a0ab8f32720c907
-- 
2.43.0