[PATCH v7 0/7] scsi: mvumi: bound the values the firmware supplies

[Yehyeong Lee]

This supersedes

  [PATCH v6 0/5] scsi: mvumi: bound the values the firmware supplies
  <20261014162413.3649841-1-yhlee@xxxxxxxxxxxxxxxxxx>

which can be dropped.

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 and the value that is used are separate fetches of
    firmware-writable memory.
 7  unrelated to the above: mvumi_delete_internal_cmd() rebuilds a DMA
    address with a shift that evaluates to zero.

I have no mvumi hardware, so none of this is tested on a controller.  Each
patch's reasoning is from the source, and I modelled the arithmetic, the
loop bounds, the index ranges and the bit numbering of every case in
userspace, including the values that should still be accepted.

One thing in mvumi_proc_msg() I could not settle and did not touch: the
second bitmap starts at param->bitmap[size >> 3].  If the firmware packs
the two bitmaps back to back with no padding, bit 0 of the second one is at
bit size % 8 of that byte, and the code reads it as bit 0.  If instead each
bitmap starts on a byte boundary, the second one starts at
DIV_ROUND_UP(size, 8) and the code is a byte early.  Either way the code is
only self-consistent when size is a multiple of 8, and I found nothing in
the tree that says which layout the firmware uses -- no documentation, no
MAINTAINERS entry, and this is the only place in the kernel with this
shape.  Rather than guess I left it, and patch 4 keeps the existing
numbering exactly.

v7:
- patch 5 reads every field of the handshake page with READ_ONCE(), not
  just the three shift counts, so a rematerialized load cannot slip a
  device count above 255 past the bound that rejects it.
- patch 6 re-checks request_id against the command the copied tag selects,
  where the controller provides one.  A tag changed between the check and
  the copy can land on a different live command, and then only request_id
  tells the two apart.
- new patch 4: walk the hotplug bitmap a byte at a time instead of casting
  it to unsigned long *, which the four-byte bitmap offset does not align.
- new patch 7: mvumi_delete_internal_cmd() shifted a u32 by 16 twice, so
  the upper half of the buffer address was dropped before
  dma_free_coherent() saw it.
- patches 1, 2 and 3 are unchanged.

Yehyeong Lee (7):
  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()

 drivers/scsi/mvumi.c | 238 ++++++++++++++++++++++++++++++++++---------
 drivers/scsi/mvumi.h |   9 ++
 2 files changed, 197 insertions(+), 50 deletions(-)


base-commit: 24a19b953e2a3d0ea5a8c4951a0ab8f32720c907
-- 
2.43.0