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

[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@isslab.korea.ac.kr>

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

[PATCH v7 1/7] scsi: mvumi: bound the per-event parameter and sense dumps

[Yehyeong Lee]

mvumi_show_event() prints param_count u32 words from params[] and
sense_data_length bytes from sense_data[], but both counts come from the
firmware event (struct mvumi_driver_event) and are never checked against
the arrays, which hold four words and 30 bytes.  A firmware that reports
the maximum (255 each) makes the loops read far past the event and past
the 512-byte buffer the event block is allocated in, printing the result
to the kernel log.

Stop each loop at the size of its array.

Fixes: f0c568a478f0 ("[SCSI] mvumi: Add Marvell UMI driver")
Cc: stable@vger.kernel.org
Signed-off-by: Yehyeong Lee <yhlee@isslab.korea.ac.kr>
---
 drivers/scsi/mvumi.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/scsi/mvumi.c b/drivers/scsi/mvumi.c
index e70d336b4ab39..38a92f8c25f5b 100644
--- a/drivers/scsi/mvumi.c
+++ b/drivers/scsi/mvumi.c
@@ -1361,7 +1361,8 @@ static void mvumi_show_event(struct mvumi_hba *mhba,
 	if (ptr->param_count) {
 		printk(KERN_WARNING "Event param(len 0x%x): ",
 						ptr->param_count);
-		for (i = 0; i < ptr->param_count; i++)
+		for (i = 0; i < ptr->param_count &&
+		     i < ARRAY_SIZE(ptr->params); i++)
 			printk(KERN_WARNING "0x%x ", ptr->params[i]);
 
 		printk(KERN_WARNING "\n");
@@ -1370,7 +1371,8 @@ static void mvumi_show_event(struct mvumi_hba *mhba,
 	if (ptr->sense_data_length) {
 		printk(KERN_WARNING "Event sense data(len 0x%x): ",
 						ptr->sense_data_length);
-		for (i = 0; i < ptr->sense_data_length; i++)
+		for (i = 0; i < ptr->sense_data_length &&
+		     i < sizeof(ptr->sense_data); i++)
 			printk(KERN_WARNING "0x%x ", ptr->sense_data[i]);
 		printk(KERN_WARNING "\n");
 	}
-- 
2.43.0

[PATCH v7 2/7] scsi: mvumi: reject an out-of-range handshake frame length

[Yehyeong Lee]

mvumi_hs_process_page() checksums and parses a handshake page using
frame_length from the page header, a u16 the firmware fills in, without
checking it against the HSP_MAX_SIZE buffer the page lives in.
mvumi_calculate_checksum() then walks frame_length bytes of frame_content
with an unsigned char loop counter, so a frame_length of 256 or more never
terminates and the probe path hangs; values between the buffer size and
255 read past the page.

Reject a frame_length that does not fit the handshake buffer before using
it.

Read frame_length once into a local and use it for both the bound check
and the checksum, so the device cannot change the value in the DMA page
between the two, and use READ_ONCE() so the single fetch is guaranteed
rather than left to the compiler.

Fixes: f0c568a478f0 ("[SCSI] mvumi: Add Marvell UMI driver")
Cc: stable@vger.kernel.org
Signed-off-by: Yehyeong Lee <yhlee@isslab.korea.ac.kr>
---
 drivers/scsi/mvumi.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/scsi/mvumi.c b/drivers/scsi/mvumi.c
index 38a92f8c25f5b..2378b676a7a78 100644
--- a/drivers/scsi/mvumi.c
+++ b/drivers/scsi/mvumi.c
@@ -1039,9 +1039,15 @@ static int mvumi_hs_process_page(struct mvumi_hba *mhba,
 {
 	struct mvumi_hs_page1 *hs_page1;
 	unsigned char page_checksum;
+	u16 frame_length = READ_ONCE(hs_header->frame_length);
 
-	page_checksum = mvumi_calculate_checksum(hs_header,
-						hs_header->frame_length);
+	if (frame_length > HSP_MAX_SIZE - sizeof(struct mvumi_hs_header)) {
+		dev_err(&mhba->pdev->dev, "invalid handshake frame length %d\n",
+			frame_length);
+		return -1;
+	}
+
+	page_checksum = mvumi_calculate_checksum(hs_header, frame_length);
 	if (page_checksum != hs_header->checksum) {
 		dev_err(&mhba->pdev->dev, "checksum error\n");
 		return -1;
-- 
2.43.0

[PATCH v7 3/7] scsi: mvumi: bound the hotplug event bitmap

[Yehyeong Lee]

mvumi_proc_msg() takes size from the hotplug event the firmware DMAs into
the event buffer and uses it twice: as the bit count for two
find_next_zero_bit() scans, and as the byte offset of the second bitmap.
Nothing checks it against the buffer, which mvumi_get_event() allocates at
a fixed 512 bytes, and size is a u16, so a firmware reporting 65535 makes
both scans run roughly 8 KiB past the allocation and places the second
bitmap past it to begin with.

Reject a size whose bitmaps do not fit.  The second bitmap starts size bits
in and find_next_zero_bit() reads a long at a time, so that span is the
furthest either scan reaches.  Compare it against sizeof(struct
mvumi_event_req), the only length the driver ever puts in
data_transfer_length and therefore the most the firmware may write into the
shared buffer, rather than against the buffer itself: dma_alloc_coherent()
hands back zeroed memory, a zero bit in these bitmaps means a device is
present, so scanning past what the firmware wrote would add devices that do
not exist.  Read size once, since it lives in memory the firmware can still
write, and give the buffer size a name while here.

Fixes: bd756ddea18e ("[SCSI] mvumi: Add support for Marvell SAS/SATA RAID-on-Chip(ROC) 88RC9580")
Cc: stable@vger.kernel.org
Signed-off-by: Yehyeong Lee <yhlee@isslab.korea.ac.kr>
---
 drivers/scsi/mvumi.c | 18 ++++++++++++++++--
 drivers/scsi/mvumi.h |  3 +++
 2 files changed, 19 insertions(+), 2 deletions(-)

diff --git a/drivers/scsi/mvumi.c b/drivers/scsi/mvumi.c
index 2378b676a7a78..9a3dde53dea97 100644
--- a/drivers/scsi/mvumi.c
+++ b/drivers/scsi/mvumi.c
@@ -1652,11 +1652,25 @@ static int mvumi_rescan_bus(void *data)
 static void mvumi_proc_msg(struct mvumi_hba *mhba,
 					struct mvumi_hotplug_event *param)
 {
-	u16 size = param->size;
+	u16 size = READ_ONCE(param->size);
 	const unsigned long *ar_bitmap;
 	const unsigned long *re_bitmap;
 	int index;
 
+	/*
+	 * The second bitmap starts size bits into the event buffer and
+	 * find_next_zero_bit() reads it a long at a time, so that is the
+	 * furthest the two scans reach.  Bound it by the length the firmware
+	 * was told it may write, not by the buffer: the bytes past that are
+	 * still zero, and a zero bit here means a device is present.
+	 */
+	if (sizeof(*param) + (size >> 3) + BITS_TO_LONGS(size) * sizeof(long) >
+			sizeof(struct mvumi_event_req)) {
+		dev_err(&mhba->pdev->dev,
+			"invalid hotplug event bitmap size %u\n", size);
+		return;
+	}
+
 	if (mhba->fw_flag & MVUMI_FW_ATTACH) {
 		index = -1;
 		ar_bitmap = (const unsigned long *) param->bitmap;
@@ -1708,7 +1722,7 @@ static int mvumi_get_event(struct mvumi_hba *mhba, unsigned char msg)
 	struct mvumi_cmd *cmd;
 	struct mvumi_msg_frame *frame;
 
-	cmd = mvumi_create_internal_cmd(mhba, 512);
+	cmd = mvumi_create_internal_cmd(mhba, MVUMI_EVENT_BUF_SIZE);
 	if (!cmd)
 		return -1;
 	cmd->scmd = NULL;
diff --git a/drivers/scsi/mvumi.h b/drivers/scsi/mvumi.h
index 1306a4abf19ad..5d45f0d99e639 100644
--- a/drivers/scsi/mvumi.h
+++ b/drivers/scsi/mvumi.h
@@ -124,6 +124,9 @@ enum {
 #define APICDB1_HOST_GETEVENT		1
 #define MAX_EVENTS_RETURNED		6
 
+/* size of the buffer the driver hands the firmware for event replies */
+#define MVUMI_EVENT_BUF_SIZE		512
+
 #define DEVICE_OFFLINE	0
 #define DEVICE_ONLINE	1
 
-- 
2.43.0

[PATCH v7 4/7] scsi: mvumi: walk the hotplug event bitmap a byte at a time

[Yehyeong Lee]

mvumi_proc_msg() casts param->bitmap to unsigned long * and hands it to
find_next_zero_bit().  bitmap sits four bytes into struct
mvumi_hotplug_event, so on a 64-bit host the pointer is four-byte aligned
where the bitmap API expects long alignment.  On an architecture that
traps unaligned loads that faults, and the second bitmap, which starts at
an arbitrary byte offset derived from size, can be misaligned on any host.

Walk the bitmap bytes instead.  The driver reads every other
firmware-supplied field natively, so bit n of the bitmap is already byte
n / 8 bit n % 8 in practice; writing that out keeps the device numbering
this driver has always used, needs no alignment from the buffer, and drops
the long-at-a-time reads the earlier bound had to allow for.  That bound
stays as it is, which is now more conservative than the walk needs.

Fixes: bd756ddea18e ("[SCSI] mvumi: Add support for Marvell SAS/SATA RAID-on-Chip(ROC) 88RC9580")
Cc: stable@vger.kernel.org
Signed-off-by: Yehyeong Lee <yhlee@isslab.korea.ac.kr>
---
 drivers/scsi/mvumi.c | 41 +++++++++++++++++------------------------
 1 file changed, 17 insertions(+), 24 deletions(-)

diff --git a/drivers/scsi/mvumi.c b/drivers/scsi/mvumi.c
index 9a3dde53dea97..18964e860ac8e 100644
--- a/drivers/scsi/mvumi.c
+++ b/drivers/scsi/mvumi.c
@@ -1653,16 +1653,15 @@ static void mvumi_proc_msg(struct mvumi_hba *mhba,
 					struct mvumi_hotplug_event *param)
 {
 	u16 size = READ_ONCE(param->size);
-	const unsigned long *ar_bitmap;
-	const unsigned long *re_bitmap;
+	const u8 *ar_bitmap;
+	const u8 *re_bitmap;
 	int index;
 
 	/*
-	 * The second bitmap starts size bits into the event buffer and
-	 * find_next_zero_bit() reads it a long at a time, so that is the
-	 * furthest the two scans reach.  Bound it by the length the firmware
-	 * was told it may write, not by the buffer: the bytes past that are
-	 * still zero, and a zero bit here means a device is present.
+	 * The second bitmap starts size bits into the event buffer, so that
+	 * is the furthest the two walks reach.  Bound it by the length the
+	 * firmware was told it may write, not by the buffer: the bytes past
+	 * that are still zero, and a zero bit here means a device is present.
 	 */
 	if (sizeof(*param) + (size >> 3) + BITS_TO_LONGS(size) * sizeof(long) >
 			sizeof(struct mvumi_event_req)) {
@@ -1672,25 +1671,19 @@ static void mvumi_proc_msg(struct mvumi_hba *mhba,
 	}
 
 	if (mhba->fw_flag & MVUMI_FW_ATTACH) {
-		index = -1;
-		ar_bitmap = (const unsigned long *) param->bitmap;
-		re_bitmap = (const unsigned long *) &param->bitmap[size >> 3];
+		ar_bitmap = param->bitmap;
+		re_bitmap = &param->bitmap[size >> 3];
 
 		mutex_lock(&mhba->sas_discovery_mutex);
-		do {
-			index = find_next_zero_bit(ar_bitmap, size, index + 1);
-			if (index >= size)
-				break;
-			mvumi_handle_hotplug(mhba, index, DEVICE_ONLINE);
-		} while (1);
-
-		index = -1;
-		do {
-			index = find_next_zero_bit(re_bitmap, size, index + 1);
-			if (index >= size)
-				break;
-			mvumi_handle_hotplug(mhba, index, DEVICE_OFFLINE);
-		} while (1);
+		for (index = 0; index < size; index++)
+			if (!(ar_bitmap[index >> 3] & (1 << (index & 7))))
+				mvumi_handle_hotplug(mhba, index,
+						     DEVICE_ONLINE);
+
+		for (index = 0; index < size; index++)
+			if (!(re_bitmap[index >> 3] & (1 << (index & 7))))
+				mvumi_handle_hotplug(mhba, index,
+						     DEVICE_OFFLINE);
 		mutex_unlock(&mhba->sas_discovery_mutex);
 	}
 }
-- 
2.43.0

[PATCH v7 5/7] scsi: mvumi: reject handshake sizes that overflow the list allocations

[Yehyeong Lee]

mvumi_hs_process_page() takes max_io, the two command entry sizes and the
list depth from the firmware handshake page.  The entry sizes are u8 fields
used as shift counts, and 1 is an int, so a count of 29 or more makes the
<< 2 applied to the shift overflow an int; a count of 32 or more is
undefined outright.  max_io is a u16, and the sizes derived from the shifts
are multiplied by it in mvumi_init_data() to size three unsigned int
allocations, so a large entry size wraps the product and the loops that
follow then walk and write past the short allocation.  The list depth is
used as a shift count too, but only when the firmware sets
HS_CAPABILITY_NEW_PAGE_IO_DEPTH_DEF; a count of 32 or more is undefined
there as well.  Nothing bounds the entry sizes from below either, and an
entry has to hold what the driver puts in it: a command frame and at least
one scatter-gather element going in, a response frame and sense data coming
back.  An entry size of 32 bytes or less makes mvumi_internal_cmd_sgl()
write its element past an inbound entry, since it writes one whatever
host->sg_tablesize says, and makes mvumi_complete_cmd() read sense data
past an outbound one; it also makes the max_sg subtraction in
mvumi_io_attach() wrap, which hands mvumi_build_frame() and
mvumi_send_command() a table size no entry can hold.  The depth also sets
how many entries the inbound and outbound rings hold, and the per-command
slot index wraps on that count while the rings are allocated for max_io
entries, so a depth yielding more entries than max_io -- or none -- walks
the index past them.  max_devices_support sizes the target_map bitmap in
the same allocation, and it is a u16 while mvumi_build_frame() packs the
target id into the low byte of device_id and the LUN into the high byte, so
a target of 256 or more aliases another device's LUN; it is also what makes
the unsigned char loop counter in mvumi_flush_cache() wrap before it
reaches the bound, which never ends that loop.  A host reset runs the
handshake again, but mvumi_init_data() returns early once MVUMI_FW_ALLOC is
set, so the buffers keep the sizes they were allocated with while the newly
reported values drive the indexing: mvumi_flush_cache() then reads
target_map past its end and mvumi_sdev_configure() sets a bit past it.  The
handshake is retried as well, because mvumi_handshake_event() discards the
error, so a page that was already rejected can be presented again.

Reject an entry size above 29, which keeps 1U << n and the shift that
follows it inside an unsigned int, and reject one too small to hold those
frames.  Reject a device count the frame layout cannot address, which also
keeps mvumi_flush_cache() terminating.  Then compute the worst-case
allocation size in u64 at the top of mvumi_init_data() and reject it if it
does not fit an unsigned int.  That total bounds all three allocations: the
inbound list total, the outbound data pool and the tag arrays, whose sizes
are the same products plus smaller terms.  Reject a list depth of 32 or
more on the shifted path so the shift stays defined, and reject a resulting
entry count that is zero or above max_io so the slot index cannot walk past
the rings.  The shifts also use 1U now so that a legal count cannot
overflow an int.  Take every field the page carries into a local with
READ_ONCE(), and run every check and every assignment on those.  The page
is coherent DMA memory, so a second fetch of a field is not the value that
was checked: a second fetch of an entry size could turn a count of 29 into
one of 31, a second fetch of the list depth could turn 31 into 32, which is
the undefined shift the check exists to stop, and a second fetch of the
device count could put a value above 255 into mhba->max_target_id after the
bound above has passed.  Once MVUMI_FW_ALLOC is set the buffers already
exist, so only check that no reported size, max_devices_support included,
is larger than the one its buffer was allocated for, and leave the size
fields alone: they already hold the sizes those buffers have.  Only the
first handshake, whose values size the allocations, assigns them.

No realistic controller is rejected: one reporting 4096 commands with 64
KiB entries and a depth of 12 needs 512 MiB and passes unchanged.  The
device count is the one new bound that could turn a controller away, and a
controller reporting more than 255 devices is already one this driver
cannot address.

Fixes: f0c568a478f0 ("[SCSI] mvumi: Add Marvell UMI driver")
Cc: stable@vger.kernel.org
Signed-off-by: Yehyeong Lee <yhlee@isslab.korea.ac.kr>
---
 drivers/scsi/mvumi.c | 124 ++++++++++++++++++++++++++++++++++++++-----
 drivers/scsi/mvumi.h |   6 +++
 2 files changed, 117 insertions(+), 13 deletions(-)

diff --git a/drivers/scsi/mvumi.c b/drivers/scsi/mvumi.c
index 18964e860ac8e..8b8e5545747e8 100644
--- a/drivers/scsi/mvumi.c
+++ b/drivers/scsi/mvumi.c
@@ -912,10 +912,23 @@ static int mvumi_init_data(struct mvumi_hba *mhba)
 	unsigned int tmp_size, offset, i;
 	void *virmem, *v;
 	dma_addr_t p;
+	u64 total;
 
 	if (mhba->fw_flag & MVUMI_FW_ALLOC)
 		return 0;
 
+	/* These are firmware-supplied and size the allocations below. */
+	total = (u64)mhba->ib_max_size * mhba->max_io +
+		(u64)mhba->ob_max_size * mhba->max_io +
+		sizeof(struct mvumi_dyn_list_entry) * (u64)mhba->max_io +
+		160;
+	if (total > UINT_MAX) {
+		dev_err(&mhba->pdev->dev,
+			"handshake sizes too large: io %u, in %u, out %u\n",
+			mhba->max_io, mhba->ib_max_size, mhba->ob_max_size);
+		return -1;
+	}
+
 	tmp_size = mhba->ib_max_size * mhba->max_io;
 	if (mhba->hba_capability & HS_CAPABILITY_SUPPORT_DYN_SRC)
 		tmp_size += sizeof(struct mvumi_dyn_list_entry) * mhba->max_io;
@@ -1054,30 +1067,115 @@ static int mvumi_hs_process_page(struct mvumi_hba *mhba,
 	}
 
 	switch (hs_header->page_code) {
-	case HS_PAGE_FIRM_CAP:
+	case HS_PAGE_FIRM_CAP: {
+		unsigned int new_max_io, new_ib_max_size, new_ob_max_size;
+		unsigned int new_list_num_io;
+		unsigned short new_max_target_id, new_capability;
+		unsigned int new_max_transfer_size;
+		u8 in_sz, out_sz, list_depth;
+
 		hs_page1 = (struct mvumi_hs_page1 *) hs_header;
 
-		mhba->max_io = hs_page1->max_io_support;
-		mhba->list_num_io = hs_page1->cl_inout_list_depth;
-		mhba->max_transfer_size = hs_page1->max_transfer_size;
-		mhba->max_target_id = hs_page1->max_devices_support;
-		mhba->hba_capability = hs_page1->capability;
-		mhba->ib_max_size_setting = hs_page1->cl_in_max_entry_size;
-		mhba->ib_max_size = (1 << hs_page1->cl_in_max_entry_size) << 2;
+		/*
+		 * The page is in coherent DMA memory, so take every field
+		 * out of it once: a second fetch of a value that has already
+		 * been checked would not be the value that was checked.
+		 */
+		in_sz = READ_ONCE(hs_page1->cl_in_max_entry_size);
+		out_sz = READ_ONCE(hs_page1->cl_out_max_entry_size);
+		list_depth = READ_ONCE(hs_page1->cl_inout_list_depth);
+		new_max_io = READ_ONCE(hs_page1->max_io_support);
+		new_max_target_id = READ_ONCE(hs_page1->max_devices_support);
+		new_capability = READ_ONCE(hs_page1->capability);
+		new_max_transfer_size = READ_ONCE(hs_page1->max_transfer_size);
+
+		/* Firmware-supplied shift counts: (1U << n) << 2 must fit. */
+		if (in_sz > 29 || out_sz > 29) {
+			dev_err(&mhba->pdev->dev,
+				"invalid handshake entry size\n");
+			return -1;
+		}
+
+		new_ib_max_size = (1U << in_sz) << 2;
+		new_ob_max_size = (1U << out_sz) << 2;
+
+		/*
+		 * A target above MVUMI_MAX_TARGET_ID cannot be addressed: its
+		 * id would land in the LUN byte of device_id and alias another
+		 * device.  It is also what makes the loop counter in
+		 * mvumi_flush_cache() wrap before it reaches the bound.
+		 */
+		if (new_max_target_id > MVUMI_MAX_TARGET_ID) {
+			dev_err(&mhba->pdev->dev,
+				"too many devices reported: %u\n",
+				new_max_target_id);
+			return -1;
+		}
 
-		mhba->ob_max_size_setting = hs_page1->cl_out_max_entry_size;
-		mhba->ob_max_size = (1 << hs_page1->cl_out_max_entry_size) << 2;
+		/* An entry has to hold the frame the driver puts in it. */
+		if (new_ib_max_size < sizeof(struct mvumi_msg_frame) +
+					sizeof(struct mvumi_sgl) ||
+		    new_ob_max_size < sizeof(struct mvumi_rsp_frame) +
+					sizeof(struct mvumi_sense_data)) {
+			dev_err(&mhba->pdev->dev,
+				"handshake entry size too small: in %u, out %u\n",
+				new_ib_max_size, new_ob_max_size);
+			return -1;
+		}
+
+		new_list_num_io = list_depth;
+		if (new_capability & HS_CAPABILITY_NEW_PAGE_IO_DEPTH_DEF) {
+			if (list_depth >= 32) {
+				dev_err(&mhba->pdev->dev,
+					"invalid handshake list depth\n");
+				return -1;
+			}
+			new_list_num_io = 1U << list_depth;
+		}
+		/* The rings hold max_io entries but the index wraps on list_num_io. */
+		if (new_list_num_io == 0 || new_list_num_io > new_max_io) {
+			dev_err(&mhba->pdev->dev,
+				"invalid handshake list depth: %u entries for %u commands\n",
+				new_list_num_io, new_max_io);
+			return -1;
+		}
 
 		dev_dbg(&mhba->pdev->dev, "FW version:%d\n",
-						hs_page1->fw_ver.ver_build);
+				READ_ONCE(hs_page1->fw_ver.ver_build));
+
+		/* A host reset does not reallocate, so the sizes have to stay
+		 * the ones the buffers were allocated with.  Only check that
+		 * the firmware is not asking for more than they hold.
+		 */
+		if (mhba->fw_flag & MVUMI_FW_ALLOC) {
+			if (new_max_io > mhba->max_io ||
+			    new_ib_max_size > mhba->ib_max_size ||
+			    new_ob_max_size > mhba->ob_max_size ||
+			    new_list_num_io > mhba->list_num_io ||
+			    new_max_target_id > mhba->max_target_id) {
+				dev_err(&mhba->pdev->dev,
+					"handshake sizes grew across reset\n");
+				return -1;
+			}
+			break;
+		}
+
+		mhba->max_io = new_max_io;
+		mhba->list_num_io = new_list_num_io;
+		mhba->max_transfer_size = new_max_transfer_size;
+		mhba->max_target_id = new_max_target_id;
+		mhba->hba_capability = new_capability;
+		mhba->ib_max_size_setting = in_sz;
+		mhba->ib_max_size = new_ib_max_size;
+		mhba->ob_max_size_setting = out_sz;
+		mhba->ob_max_size = new_ob_max_size;
 
 		if (mhba->hba_capability & HS_CAPABILITY_SUPPORT_COMPACT_SG)
 			mhba->eot_flag = 22;
 		else
 			mhba->eot_flag = 27;
-		if (mhba->hba_capability & HS_CAPABILITY_NEW_PAGE_IO_DEPTH_DEF)
-			mhba->list_num_io = 1 << hs_page1->cl_inout_list_depth;
 		break;
+	}
 	default:
 		dev_err(&mhba->pdev->dev, "handshake: page code error\n");
 		return -1;
diff --git a/drivers/scsi/mvumi.h b/drivers/scsi/mvumi.h
index 5d45f0d99e639..d20eea2b61a31 100644
--- a/drivers/scsi/mvumi.h
+++ b/drivers/scsi/mvumi.h
@@ -127,6 +127,12 @@ enum {
 /* size of the buffer the driver hands the firmware for event replies */
 #define MVUMI_EVENT_BUF_SIZE		512
 
+/*
+ * mvumi_build_frame() packs the target id into the low byte of device_id
+ * and the LUN into the high byte, so only these ids can be addressed.
+ */
+#define MVUMI_MAX_TARGET_ID		255
+
 #define DEVICE_OFFLINE	0
 #define DEVICE_ONLINE	1
 
-- 
2.43.0

Re: [PATCH v7 5/7] scsi: mvumi: reject handshake sizes that overflow the list allocations

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] scsi: mvumi: newly added handshake bounds check breaks initialization on big-endian systems
--

commit 36e8afa776de38b656fa4baf03b7793afdcd3fef
Author: Yehyeong Lee <yhlee@isslab.korea.ac.kr>

scsi: mvumi: reject handshake sizes that overflow the list allocations

This patch adds missing bounds checks in mvumi_hs_process_page() to reject
invalid hardware handshake sizes. It prevents buffer overflows when the
hardware reports excessively large list allocations, capability fields, or
device limits.

> diff --git a/drivers/scsi/mvumi.c b/drivers/scsi/mvumi.c
> index 18964e860ac8e..8b8e5545747e8 100644
> --- a/drivers/scsi/mvumi.c
> +++ b/drivers/scsi/mvumi.c
[ ... ]
> @@ -1054,30 +1067,115 @@ static int mvumi_hs_process_page(struct mvumi_hba *mhba,
[ ... ]
> +		/*
> +		 * The page is in coherent DMA memory, so take every field
> +		 * out of it once: a second fetch of a value that has already
> +		 * been checked would not be the value that was checked.
> +		 */
> +		in_sz = READ_ONCE(hs_page1->cl_in_max_entry_size);
> +		out_sz = READ_ONCE(hs_page1->cl_out_max_entry_size);
> +		list_depth = READ_ONCE(hs_page1->cl_inout_list_depth);
> +		new_max_io = READ_ONCE(hs_page1->max_io_support);
> +		new_max_target_id = READ_ONCE(hs_page1->max_devices_support);

[Severity: High]
Since hs_page1 is populated by the hardware via DMA, aren't these fields
inherently little-endian?

Reading hs_page1->max_devices_support directly into new_max_target_id without
le16_to_cpu() causes the bytes to be swapped on big-endian architectures.

> +		new_capability = READ_ONCE(hs_page1->capability);
> +		new_max_transfer_size = READ_ONCE(hs_page1->max_transfer_size);
> +
[ ... ]
> +		/*
> +		 * A target above MVUMI_MAX_TARGET_ID cannot be addressed: its
> +		 * id would land in the LUN byte of device_id and alias another
> +		 * device.  It is also what makes the loop counter in
> +		 * mvumi_flush_cache() wrap before it reaches the bound.
> +		 */
> +		if (new_max_target_id > MVUMI_MAX_TARGET_ID) {

[Severity: High]
Will this newly added bounds check incorrectly fail the probe on big-endian
systems?

A valid firmware-reported target count of 1 (0x0001) would be byte-swapped
to 256 (0x0100). Since MVUMI_MAX_TARGET_ID is 255, this check would evaluate
256 > 255 as true, falsely log "too many devices reported", and abort the
initialization by returning -1.

> +			dev_err(&mhba->pdev->dev,
> +				"too many devices reported: %u\n",
> +				new_max_target_id);
> +			return -1;
> +		}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261015005717.3719956-1-yhlee@isslab.korea.ac.kr?part=5

[PATCH v7 6/7] scsi: mvumi: validate the response frame tag before using it

[Yehyeong Lee]

mvumi_handle_clob() indexes mhba->tag_cmd[] with the tag of a response
frame the firmware DMAs into the outbound list.  The array holds
tag_pool.size entries, and the only check on that tag runs earlier, in
mvumi_receive_ob_list_entry() and mvumi_check_ob_frame(), against the frame
still sitting in the outbound list.  mvumi_receive_ob_list_entry() then
copies the frame into an ob_data buffer and mvumi_handle_clob() reads the
tag out of that copy, so the value that is checked and the value that is
used are two separate fetches of firmware-writable memory.  A firmware that
changes the tag after the check reaches tag_cmd[] with any u16, reads a
pointer past the array and dereferences it, writes NULL past the array and
pushes the tag onto the free stack for a later command to pick up.  The
check is also off by one: tag_cmd[] and tag_pool.stack[] both hold
tag_pool.size entries, so a tag equal to tag_pool.size passes a > test and
lands one entry past both.

Check the tag that mvumi_handle_clob() actually uses, against the size of
the array it indexes, and check the command it finds for NULL before
dereferencing it: with two fetches the earlier NULL check no longer says
anything about this one.  Re-check request_id against that command too,
where the controller provides one, for the same reason the earlier check
does: a tag changed between the check and the copy can land on a different
command that is also live, and then only request_id tells the two apart.
Make the two existing bounds checks >= so a tag equal to the entry count is
rejected there too, and read the tag into a local in
mvumi_receive_ob_list_entry() as well, so the check there and the accesses
beside it see one value instead of three fetches.  Use READ_ONCE() for the
tag and request_id both functions take out of the outbound list, so the
single fetch is guaranteed rather than left to the compiler.

Fixes: f0c568a478f0 ("[SCSI] mvumi: Add Marvell UMI driver")
Cc: stable@vger.kernel.org
Signed-off-by: Yehyeong Lee <yhlee@isslab.korea.ac.kr>
---
 drivers/scsi/mvumi.c | 45 ++++++++++++++++++++++++++++++++++----------
 1 file changed, 35 insertions(+), 10 deletions(-)

diff --git a/drivers/scsi/mvumi.c b/drivers/scsi/mvumi.c
index 8b8e5545747e8..2410d2aa147d9 100644
--- a/drivers/scsi/mvumi.c
+++ b/drivers/scsi/mvumi.c
@@ -466,9 +466,9 @@ static char mvumi_check_ob_frame(struct mvumi_hba *mhba,
 
 	udelay(1);
 	p_outb_frame = mhba->ob_list + cur_obf * mhba->ob_max_size;
-	request_id = p_outb_frame->request_id;
-	tag = p_outb_frame->tag;
-	if (tag > mhba->tag_pool.size) {
+	request_id = READ_ONCE(p_outb_frame->request_id);
+	tag = READ_ONCE(p_outb_frame->tag);
+	if (tag >= mhba->tag_pool.size) {
 		dev_err(&mhba->pdev->dev, "ob frame data error\n");
 		return -1;
 	}
@@ -530,6 +530,7 @@ static void mvumi_receive_ob_list_entry(struct mvumi_hba *mhba)
 	struct mvumi_ob_data *ob_data;
 	struct mvumi_rsp_frame *p_outb_frame;
 	struct mvumi_hw_regs *regs = mhba->regs;
+	unsigned short tag;
 
 	if (mhba->instancet->check_ob_list(mhba, &cur_obf, &assign_obf_end))
 		return;
@@ -546,10 +547,11 @@ static void mvumi_receive_ob_list_entry(struct mvumi_hba *mhba)
 		/* Copy pointer may point to entry in outbound list
 		*  before entry has valid data
 		*/
-		if (unlikely(p_outb_frame->tag > mhba->tag_pool.size ||
-			mhba->tag_cmd[p_outb_frame->tag] == NULL ||
-			p_outb_frame->request_id !=
-				mhba->tag_cmd[p_outb_frame->tag]->request_id))
+		tag = READ_ONCE(p_outb_frame->tag);
+		if (unlikely(tag >= mhba->tag_pool.size ||
+			     !mhba->tag_cmd[tag] ||
+			     mhba->tag_cmd[tag]->request_id !=
+			     READ_ONCE(p_outb_frame->request_id)))
 			if (mvumi_check_ob_frame(mhba, cur_obf, p_outb_frame))
 				continue;
 
@@ -1878,6 +1880,7 @@ static void mvumi_handle_clob(struct mvumi_hba *mhba)
 	struct mvumi_rsp_frame *ob_frame;
 	struct mvumi_cmd *cmd;
 	struct mvumi_ob_data *pool;
+	unsigned short tag;
 
 	while (!list_empty(&mhba->free_ob_list)) {
 		pool = list_first_entry(&mhba->free_ob_list,
@@ -1886,11 +1889,33 @@ static void mvumi_handle_clob(struct mvumi_hba *mhba)
 		list_add_tail(&pool->list, &mhba->ob_data_list);
 
 		ob_frame = (struct mvumi_rsp_frame *) &pool->data[0];
-		cmd = mhba->tag_cmd[ob_frame->tag];
+		/*
+		 * The tag was checked against the frame in the outbound list.
+		 * This is the copy, so check the value that is used.
+		 */
+		tag = ob_frame->tag;
+		if (tag >= mhba->tag_pool.size) {
+			dev_err(&mhba->pdev->dev,
+				"ob frame tag[0x%x] out of range\n", tag);
+			continue;
+		}
+		cmd = mhba->tag_cmd[tag];
+		if (!cmd) {
+			dev_err(&mhba->pdev->dev,
+				"ob frame tag[0x%x] with no command\n", tag);
+			continue;
+		}
+		if (mhba->request_id_enabled &&
+		    ob_frame->request_id != cmd->request_id) {
+			dev_err(&mhba->pdev->dev,
+				"ob frame tag[0x%x] request ID 0x%x != 0x%x\n",
+				tag, ob_frame->request_id, cmd->request_id);
+			continue;
+		}
 
 		atomic_dec(&mhba->fw_outstanding);
-		mhba->tag_cmd[ob_frame->tag] = NULL;
-		tag_release_one(mhba, &mhba->tag_pool, ob_frame->tag);
+		mhba->tag_cmd[tag] = NULL;
+		tag_release_one(mhba, &mhba->tag_pool, tag);
 		if (cmd->scmd)
 			mvumi_complete_cmd(mhba, cmd, ob_frame);
 		else
-- 
2.43.0

Re: [PATCH v7 6/7] scsi: mvumi: validate the response frame tag before using it

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] scsi: mvumi: unconditional 1us delay per completion for MV9580
- [High] scsi: mvumi: missing endianness conversion for PCI DMA structure fields
- [High] scsi: mvumi: tombstone tag 0xff collides with valid tags if max_io > 255
- [Medium] scsi: mvumi: missing dma_rmb() before reading DMA payload
--

commit fe020b1f81f0a368740581377e4d8b32ba6b98b3
Author: Yehyeong Lee <yhlee@isslab.korea.ac.kr>

scsi: mvumi: validate the response frame tag before using it

This commit fixes out-of-bounds array access and time-of-check to time-of-use
issues in the mvumi driver by ensuring the response frame tag and request ID
are fetched using READ_ONCE() before validation. It also adjusts tag bounds
checks to handle off-by-one errors and checks the tag against the array size it
indexes.

> diff --git a/drivers/scsi/mvumi.c b/drivers/scsi/mvumi.c
> index 8b8e5545747e8..2410d2aa147d9 100644
> --- a/drivers/scsi/mvumi.c
> +++ b/drivers/scsi/mvumi.c
> @@ -466,9 +466,9 @@ static char mvumi_check_ob_frame(struct mvumi_hba *mhba,
>  
>  	udelay(1);
>  	p_outb_frame = mhba->ob_list + cur_obf * mhba->ob_max_size;
> -	request_id = p_outb_frame->request_id;
> -	tag = p_outb_frame->tag;
> -	if (tag > mhba->tag_pool.size) {
> +	request_id = READ_ONCE(p_outb_frame->request_id);
> +	tag = READ_ONCE(p_outb_frame->tag);
> +	if (tag >= mhba->tag_pool.size) {

[Severity: High]
Since the hardware writes response frames to DMA memory in little-endian
format, does reading tag and request_id directly as native unsigned short
fields cause valid response frames to fail bounds checks on big-endian
architectures?

>  		dev_err(&mhba->pdev->dev, "ob frame data error\n");
>  		return -1;
>  	}
>  

[ ... ]

> @@ -546,10 +547,11 @@ static void mvumi_receive_ob_list_entry(struct mvumi_hba *mhba)
>  		/* Copy pointer may point to entry in outbound list
>  		*  before entry has valid data
>  		*/
> -		if (unlikely(p_outb_frame->tag > mhba->tag_pool.size ||
> -			mhba->tag_cmd[p_outb_frame->tag] == NULL ||
> -			p_outb_frame->request_id !=
> -				mhba->tag_cmd[p_outb_frame->tag]->request_id))
> +		tag = READ_ONCE(p_outb_frame->tag);
> +		if (unlikely(tag >= mhba->tag_pool.size ||
> +			     !mhba->tag_cmd[tag] ||
> +			     mhba->tag_cmd[tag]->request_id !=
> +			     READ_ONCE(p_outb_frame->request_id)))

[Severity: High]
For MV9580 controllers where request_id_enabled is 0, will this condition
almost always evaluate to true because the hardware does not reliably
populate request_id?

If so, this forces a fallback to mvumi_check_ob_frame() for every command
completion, causing an unconditional udelay(1) per command.

[Severity: Medium]
Does this code need a dma_rmb() before reading the DMA payload to ensure
weakly-ordered architectures don't speculatively execute the memcpy() payload
fetch before the preceding tag and request_id validity check completes?

Later in mvumi_receive_ob_list_entry(), the driver reads the payload:

        memcpy(ob_data->data, p_outb_frame, mhba->ob_max_size);

Without a memory barrier, fetching stale DMA payload data (which includes I/O
completion status and sense data) can result in incorrect I/O completion
handling or data corruption.

[Severity: High]
If the firmware negotiates a max_io greater than 255 (such as 512 or 1024),
could the hardcoded tombstone value of 0xff collide with a legitimate tag?

Further down in mvumi_receive_ob_list_entry(), the code does:

        p_outb_frame->tag = 0xff;

The hardware's copy pointer can advance before the DMA completes, meaning the
driver could read a stale tombstoned slot. If it sees tag 255, and tag 255 is
currently active with a matching request_id, the unlikely condition evaluates
to false. This bypasses the mvumi_check_ob_frame() delay loop entirely,
causing the driver to prematurely complete the active command with garbage
data.

>  			if (mvumi_check_ob_frame(mhba, cur_obf, p_outb_frame))
>  				continue;
>

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261015005717.3719956-1-yhlee@isslab.korea.ac.kr?part=6

Re: [PATCH v7 6/7] scsi: mvumi: validate the response frame tag before using it

[Yehyeong Lee]

Thanks for the review. None of these are regressions introduced by this
series; I verified each against the base tree.

Endianness (this patch and 5/7): the driver does no device-to-host endian
conversion anywhere -- these fields are plain u16 and are used natively
throughout (e.g. 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. I'll note that on a big-endian
build the base driver silently accepts a byte-swapped count and scans
phantom targets; patch 5 turns that into an explicit probe failure.

MV9580 / per-completion udelay: the unconditional request_id comparison in
mvumi_receive_ob_list_entry() is pre-existing. This patch only wraps the
reads in READ_ONCE(), fixes the > to >= off-by-one, and localizes the tag;
the comparison itself is unchanged. The new re-check I add in the
completion handler is gated by request_id_enabled, matching the existing
gate 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 here 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 notes an
entry "may point to [an] entry in [the] outbound list before [the] 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 CPU barrier cannot make a
not-yet-DMAed payload arrive. I don't think a dma_rmb() here is correct
without a firmware write-ordering guarantee I can't establish from the
tree.

tombstone tag 0xff vs max_io > 255: correct, and pre-existing --
max_io_support is not bounded and the 0xff sentinel can alias a valid tag.
This series does not introduce it (the re-check reads the dequeued copy
before the sentinel is written), but the defect is real; I'll send a
follow-up that stamps the consumed entry with 0xffff, which is always
outside the u16 tag range, once this series lands.

[PATCH v7 7/7] scsi: mvumi: fix the DMA address rebuilt in mvumi_delete_internal_cmd()

[Yehyeong Lee]

mvumi_internal_cmd_sgl() splits the address of the internal command's data
buffer across two u32 fields, lower_32_bits() in baseaddr_l and
upper_32_bits() in baseaddr_h.  mvumi_delete_internal_cmd() puts it back
together with

	phy_addr = (dma_addr_t) m_sg->baseaddr_l |
		(dma_addr_t) ((m_sg->baseaddr_h << 16) << 16);

but baseaddr_h is a u32, so both shifts happen in 32-bit arithmetic and the
result is zero.  Every byte of the upper half is dropped, and on a 64-bit
host whose DMA mask let the buffer land above 4G, dma_free_coherent() is
handed an address that is not the one it returned.

Shift in u64 and cast once.  Where dma_addr_t is 32 bits the cast truncates
to what lower_32_bits() stored, and upper_32_bits() stored zero, so the
result is unchanged there.

Fixes: f0c568a478f0 ("[SCSI] mvumi: Add Marvell UMI driver")
Cc: stable@vger.kernel.org
Signed-off-by: Yehyeong Lee <yhlee@isslab.korea.ac.kr>
---
 drivers/scsi/mvumi.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/scsi/mvumi.c b/drivers/scsi/mvumi.c
index 2410d2aa147d9..bd1a8c1d54aeb 100644
--- a/drivers/scsi/mvumi.c
+++ b/drivers/scsi/mvumi.c
@@ -295,8 +295,8 @@ static void mvumi_delete_internal_cmd(struct mvumi_hba *mhba,
 			m_sg = (struct mvumi_sgl *) &cmd->frame->payload[0];
 			sgd_getsz(mhba, m_sg, size);
 
-			phy_addr = (dma_addr_t) m_sg->baseaddr_l |
-				(dma_addr_t) ((m_sg->baseaddr_h << 16) << 16);
+			phy_addr = (dma_addr_t)(((u64)m_sg->baseaddr_h << 32) |
+						m_sg->baseaddr_l);
 
 			dma_free_coherent(&mhba->pdev->dev, size, cmd->data_buf,
 								phy_addr);
-- 
2.43.0