[PATCH v6 0/5] scsi: mvumi: bound the values the firmware supplies

[PATCH v6 0/5] scsi: mvumi: bound the values the firmware supplies

[Yehyeong Lee]

This supersedes

  [PATCH v5 0/6] scsi: mvumi: bound the values the firmware supplies
  <20261014151914.3632701-1-yhlee@isslab.korea.ac.kr>

which can be dropped, and with it the two threads v5 already replaced:

  [PATCH v2 0/2] scsi: mvumi: bound two firmware-supplied lengths
  <20261013173145.3412863-1-yhlee@isslab.korea.ac.kr>

  [PATCH v4] scsi: mvumi: reject handshake sizes that overflow the list
  allocations
  <20261014134104.3528958-1-yhlee@isslab.korea.ac.kr>

Five patches instead of six: the widening of the mvumi_flush_cache() loop
counter is gone, replaced by a bound on the device count in the handshake
patch, and I owe a correction on v5 4/6 (below).

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 find_next_zero_bit()
    scans with nothing holding it inside what the firmware was asked for.
 4  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, and a reset reports all of them again
    without reallocating anything.
 5  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.

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 and the index ranges of every case in userspace, including the
values that should still be accepted.

A correction on v5 4/6: its commit message said it rejected an entry size
too small to hold the frames the driver puts in the entry.  The code did
not -- I lost that hunk before posting and did not catch it.  Patch 4 here
carries both the check and that wording.

v6:
- 4/6 "widen the flush cache loop counter" is dropped.  Patch 4 instead
  rejects a device count above 255, which is what mvumi_build_frame() can
  address: it 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.  The same bound is what keeps the unsigned char counter in
  mvumi_flush_cache() terminating, so that loop needs no change.  This
  replaces what I wrote in reply to the v4 review, that rejecting such a
  count would turn away a working configuration -- it would not, the frame
  layout cannot address those targets at all.
- patch 4 regains the entry-size minimum described above.
- patch 3 bounds the bitmap by sizeof(struct mvumi_event_req), the length
  the driver puts in data_transfer_length, instead of by the 512-byte
  buffer.  dma_alloc_coherent() returns zeroed memory and a zero bit means
  a device is present, so the bytes past what the firmware wrote would be
  read as devices that do not exist.
- patch 5 uses READ_ONCE() for the tag and request_id it takes out of the
  outbound list, and patch 4 does the same for the three shift counts it
  takes out of the handshake page, matching patches 2 and 3.  Checking one
  fetch of a shift count and shifting by another is how a device gets past
  the bound those checks exist for.
- patches 1 and 2 are unchanged.

Yehyeong Lee (5):
  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: reject handshake sizes that overflow the list allocations
  scsi: mvumi: validate the response frame tag before using it

 drivers/scsi/mvumi.c | 187 ++++++++++++++++++++++++++++++++++++-------
 drivers/scsi/mvumi.h |   9 +++
 2 files changed, 168 insertions(+), 28 deletions(-)


base-commit: 24a19b953e2a3d0ea5a8c4951a0ab8f32720c907
-- 
2.43.0

[PATCH v6 1/5] 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 v6 2/5] 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 v6 3/5] 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

Re: [PATCH v6 3/5] scsi: mvumi: bound the hotplug event bitmap

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] scsi: mvumi: unaligned memory access in find_next_zero_bit()
- [High] scsi: mvumi: unaligned bitmap access and overlap in hotplug event
--

commit 858b7c6c7bb4515208e9ef60baca2c850663e824
Author: Yehyeong Lee <yhlee@isslab.korea.ac.kr>
scsi: mvumi: bound the hotplug event bitmap

This commit bounds the hotplug event bitmap to prevent the firmware from
reading past the event buffer. It checks the hotplug event size against
the expected structure length.

> 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)) {

[Severity: High]
Does this code introduce an unaligned memory access and potential overlap
between bitmaps?

Looking at struct mvumi_hotplug_event, the bitmap array sits at a 4-byte
offset:

drivers/scsi/mvumi.h:
struct mvumi_hotplug_event {
	u16 size;
	u8 dummy[2];
	u8 bitmap[];
};

Since param->bitmap sits at a 4-byte offset within the DMA buffer, casting it
to unsigned long * and passing it to find_next_zero_bit() in mvumi_proc_msg()
will cause an alignment fault on strict-alignment 64-bit architectures.

Additionally, if the firmware provides a size that is not a multiple of 8,
does the size >> 3 offset truncate the remainder?

If truncated, re_bitmap will overlap with ar_bitmap's memory, which might
cause the same bits to be scanned for both device additions and removals.
This new bounds check explicitly relies on the size >> 3 offset without
accounting for the unaligned access or overlap.

> +		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;

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

[PATCH v6 4/5] 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 the page's values into locals and run every check on
those, with READ_ONCE() for the three shift counts so the value that is
checked is the value that is shifted; a second fetch of the entry size
could turn a count of 29 into one of 31, and a second fetch of the list
depth could turn 31 into 32, which is the undefined shift the check exists
to stop.  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 | 115 ++++++++++++++++++++++++++++++++++++++-----
 drivers/scsi/mvumi.h |   6 +++
 2 files changed, 109 insertions(+), 12 deletions(-)

diff --git a/drivers/scsi/mvumi.c b/drivers/scsi/mvumi.c
index 9a3dde53dea97..2871b4c96f1c8 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,108 @@ 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;
+		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;
+		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);
+
+		/* 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_max_io = hs_page1->max_io_support;
+		new_ib_max_size = (1U << in_sz) << 2;
+		new_ob_max_size = (1U << out_sz) << 2;
+		new_max_target_id = hs_page1->max_devices_support;
+		new_capability = hs_page1->capability;
+
+		/*
+		 * 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;
+		}
+
+		/* 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;
+		}
 
-		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;
+		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);
 
+		/* 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 = hs_page1->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 v6 4/5] 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: missing READ_ONCE on DMA fields causes TOCTOU
--

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

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

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.
[ ... ]

> diff --git a/drivers/scsi/mvumi.c b/drivers/scsi/mvumi.c
> index 9a3dde53dea97..2871b4c96f1c8 100644
> --- a/drivers/scsi/mvumi.c
> +++ b/drivers/scsi/mvumi.c

[ ... ]

> @@ -1054,30 +1067,108 @@ 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;
> +		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;
> +		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);
> +
> +		/* 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_max_io = hs_page1->max_io_support;
> +		new_ib_max_size = (1U << in_sz) << 2;
> +		new_ob_max_size = (1U << out_sz) << 2;
> +		new_max_target_id = hs_page1->max_devices_support;
> +		new_capability = hs_page1->capability;

[Severity: High]
This patch correctly adds READ_ONCE() for the shift counts, but is it possible
the remaining DMA-mapped fields are vulnerable to a Time-Of-Check to
Time-Of-Use (TOCTOU) race?

Because `hs_page1` resides in coherent DMA memory, the hardware can modify it
concurrently. The compiler is permitted to optimize away local variables like
`new_max_target_id` and reload them directly from `hs_page1` later in the
function.

> +
> +		/*
> +		 * 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;
> +		}
> +
> +		/* An entry has to hold the frame the driver puts in it. */

[ ... ]

> +		/* 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 = hs_page1->max_transfer_size;
> +		mhba->max_target_id = new_max_target_id;
> +		mhba->hba_capability = new_capability;

If the compiler rematerializes `new_max_target_id` by doing a read from
`hs_page1->max_devices_support` here during the assignment, could a malicious
or malfunctioning device have changed it after the bounds check?

This would cause `mhba->max_target_id` to exceed 255, leading to the target
aliasing and loop wrap-around vulnerabilities this patch intended to prevent in
mvumi_flush_cache().

Should READ_ONCE() also be used when reading `max_io_support`,
`max_devices_support`, and `capability` from `hs_page1`?

> +		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;

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

[PATCH v6 5/5] 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.  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 | 38 ++++++++++++++++++++++++++++----------
 1 file changed, 28 insertions(+), 10 deletions(-)

diff --git a/drivers/scsi/mvumi.c b/drivers/scsi/mvumi.c
index 2871b4c96f1c8..85624efcc1725 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,26 @@ 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;
+		}
 
 		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 v6 5/5] scsi: mvumi: validate the response frame tag before using it

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] scsi: mvumi: incomplete TOCTOU fix missing request_id re-validation
--

commit 603dc78073e88003a67484fc0cd705e2144c5c40
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 accesses and NULL pointer
dereferences in mvumi_handle_clob() caused by a double-fetch/TOCTOU
vulnerability. It re-validates the response frame tag and command pointer
from the copied local buffer.

> diff --git a/drivers/scsi/mvumi.c b/drivers/scsi/mvumi.c
> index 2871b4c96f1c8..85624efcc1725 100644
> --- a/drivers/scsi/mvumi.c
> +++ b/drivers/scsi/mvumi.c

[ ... ]

> @@ -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,26 @@ 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;
> +		}

[Severity: Medium]
Is a re-validation of the request_id also needed here?

The patch aims to prevent a double-fetch vulnerability by re-checking the
tag and command after copying the frame to the local ob_data buffer.
However, the request_id does not appear to be re-verified against
cmd->request_id.

If the firmware modifies the tag in the outbound list after the initial
check in mvumi_receive_ob_list_entry() but before the copy:

1. The initial validation passes with a valid tag and matching request_id.
2. The firmware changes the tag to point to a different, valid command.
3. The memcpy() grabs the modified tag into the local ob_data buffer.
4. mvumi_handle_clob() validates that the modified tag is in bounds and
   that the associated command exists.

Without verifying that ob_frame->request_id matches cmd->request_id on
this copied frame, does this regression allow the driver to blindly complete
the swapped command with the payload from this response, potentially causing
data corruption or logical errors?

>  
>  		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

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