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

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

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

[PATCH v8 1/8] 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 v8 2/8] 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

Re: [PATCH v8 2/8] scsi: mvumi: reject an out-of-range handshake frame length

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] scsi: mvumi: missing endianness conversion for frame_length
--

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

scsi: mvumi: reject an out-of-range handshake frame length

This patch adds bounds checking to the frame_length obtained from the firmware
handshake page to prevent out-of-bounds reads and hangs during checksum
calculations. It also uses READ_ONCE() to ensure the DMA value is fetched only
once.

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

[Severity: Medium]
Is an endianness conversion required here before the bounds check?

hs_header->frame_length is populated by the PCI device, which typically uses
little-endian byte order. On big-endian architectures, reading it directly into
a u16 without le16_to_cpu() results in a byte-swapped value.

This swapped value will cause the bounds check against HSP_MAX_SIZE to
spuriously fail the handshake and abort the device probe. While the driver may
lack complete endian conversions overall, does this new check introduce an
immediate probe failure on big-endian platforms?

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

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

[PATCH v8 3/8] 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 v8 3/8] scsi: mvumi: bound the hotplug event bitmap

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] scsi: mvumi: missing endianness conversion for firmware size field
--

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

scsi: mvumi: bound the hotplug event bitmap

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.
This commit rejects a size whose bitmaps do not fit within the event buffer
to prevent the driver from reading past the allocation.

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

[Severity: High]
Does this code miss an endianness conversion for the firmware size field?

The hardware firmware provides a little-endian size, so on big-endian
architectures, a valid small size (e.g., 0x0040) is natively read as a
massive byte-swapped size (0x4000) when le16_to_cpu() isn't used.

Because of the newly introduced bounds check below, this inflated value will
now fail validation, discarding the valid hotplug event entirely and rendering
the driver unable to register attached devices.

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

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

[PATCH v8 4/8] 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 v8 5/8] 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 v8 5/8] 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 firmware abort write causes infinite retry loop
--

commit 4d0f01fe304c4edb98be93f9e2717cb4c6551a92
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 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;

[Severity: High]
If these new boundary checks fail and return -1, will it cause an infinite
retry loop?

When mvumi_hs_process_page() or mvumi_init_data() returns -1,
mvumi_handshake() sets hs_fun to HS_S_ABORT but returns immediately without
writing the abort state to the doorbell registers:

	if (mvumi_hs_process_page(mhba, hs_header)) {
		HS_SET_STATE(hs_fun, HS_S_ABORT);
		return -1;
	}

Because the firmware never receives the abort, it waits for a response and
eventually retries by asserting DRBL_HANDSHAKE_ISR.

The caller mvumi_handshake_event() then processes the interrupt, calls
mvumi_handshake() again, and resets the timeout:

	if (isr_status & DRBL_HANDSHAKE_ISR) {
		mhba->instancet->clear_intr(mhba);
		mvumi_handshake(mhba);
		before = jiffies;
	}

Doesn't this result in the driver probe hanging indefinitely on bad firmware
instead of timing out or failing cleanly?

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

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

[PATCH v8 6/8] 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 v8 6/8] 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:
- [High] scsi: mvumi: missing dma_rmb() causes speculative payload reads
--

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

    scsi: mvumi: validate the response frame tag before using it
    
    This commit fixes bounds checking and validates the response frame tag
    used in the outbound list. It also ensures the tag and request ID are
    retrieved using READ_ONCE() to prevent compiler optimizations that could
    lead to multiple fetches.

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

[Severity: High]
Does this code need a dma_rmb() here?

The driver polls the completion tokens (tag and request_id) in DMA memory
here. A few lines below, the outbound DMA response frame is copied:

mvumi_receive_ob_list_entry() {
    ...
		tag = READ_ONCE(p_outb_frame->tag);
		if (unlikely(...))
			...
    ...
		memcpy(ob_data->data, p_outb_frame, mhba->ob_max_size);
    ...
}

Without a dma_rmb() after the completion tokens are validated, can the CPU
speculatively execute the memcpy() of the payload before reading the
completion token?

On weakly ordered architectures, the CPU may read stale values for
req_status or payload during the memcpy(). This could lead to silent data
corruption if a failed disk IO is incorrectly seen as successful.

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

[PATCH v8 7/8] 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

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

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] scsi: mvumi: missing endianness conversion for DMA address SGL fields
- [High] scsi: mvumi: memory corruption due to trusting DMA memory for allocator parameters
--

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

scsi: mvumi: fix the DMA address rebuilt in mvumi_delete_internal_cmd()

This commit fixes a bug in mvumi_delete_internal_cmd() where a 32-bit shift
on baseaddr_h resulted in truncation of the upper 32 bits of a DMA address.
The fix uses a 64-bit cast before shifting to ensure the address is properly
reconstructed for dma_free_coherent().

Cc: stable@vger.kernel.org

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

[Severity: High]
Does reading the size natively here cause issues on big-endian architectures?
The SGL buffer size is natively read back from a little-endian field in
device DMA memory without using le32_to_cpu().

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

[Severity: High]
Is it safe to construct this DMA address without endianness conversion?

These SGL fields are populated using cpu_to_le32() but are read back natively.
On big-endian architectures, reading little-endian SGL fields natively yields
corrupted values, which could cause dma_free_coherent() to unmap the wrong
address or trigger a panic.

[Severity: High]
Can a compromised PCIe device modify these fields concurrently?

cmd->frame is allocated in bidirectionally mapped DMA-coherent memory,
so an untrusted device could modify sg_counts, size, baseaddr_h, and baseaddr_l
after they have been initialized or concurrently during cleanup.

Since the driver uses these values directly as parameters for
dma_free_coherent() below, this introduces a DMA double fetch / TOCTOU
vulnerability that could allow arbitrary unmapping of memory.

>  
>  			dma_free_coherent(&mhba->pdev->dev, size, cmd->data_buf,
>  								phy_addr);

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

[PATCH v8 8/8] scsi: mvumi: stamp a consumed outbound list entry out of range

[Yehyeong Lee]

mvumi_receive_ob_list_entry() copies a completion out of the outbound
list and then writes 0xff over the tag, so that the entry is rejected if
the firmware's copy pointer brings the driver back to it.  The comment
right above the checks says why that can happen: "Copy pointer may point
to entry in outbound list before entry has valid data".

Nothing ever compares a tag against 0xff.  The only test for a usable
entry is

	tag >= mhba->tag_pool.size

and tag_pool.size is mhba->max_io, which the firmware supplies in the u16
max_io_support.  As soon as the firmware asks for more than 255 commands,
0xff is a tag in range and tag_cmd[255] can hold a live command, so a
stale consumed entry is no longer turned away by its tag.

What happens next depends on the controller.  Where request_id_enabled is
set, mvumi_check_ob_frame() still compares the request_id left in the
entry and rejects the frame.  Where it is not set, and the MV9580 case in
mvumi_init_fw() never sets it, that comparison is skipped, so the entry
passes, is copied, and completes whichever command owns tag 255 with the
bytes of an older completion.

The tag field is a u16 and tag_pool.size is an unsigned short, so 0xffff
is >= every value the bound can take and is never a valid tag.  Stamp
that instead.  The store is the same width it was.

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 | 2 +-
 drivers/scsi/mvumi.h | 6 ++++++
 2 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/scsi/mvumi.c b/drivers/scsi/mvumi.c
index bd1a8c1d54aeb..67a6bb6536950 100644
--- a/drivers/scsi/mvumi.c
+++ b/drivers/scsi/mvumi.c
@@ -571,7 +571,7 @@ static void mvumi_receive_ob_list_entry(struct mvumi_hba *mhba)
 		}
 
 		memcpy(ob_data->data, p_outb_frame, mhba->ob_max_size);
-		p_outb_frame->tag = 0xff;
+		p_outb_frame->tag = MVUMI_TAG_CONSUMED;
 
 		list_add_tail(&ob_data->list, &mhba->free_ob_list);
 	}
diff --git a/drivers/scsi/mvumi.h b/drivers/scsi/mvumi.h
index d20eea2b61a31..f69a0807100c3 100644
--- a/drivers/scsi/mvumi.h
+++ b/drivers/scsi/mvumi.h
@@ -133,6 +133,12 @@ enum {
  */
 #define MVUMI_MAX_TARGET_ID		255
 
+/*
+ * Stamped into a consumed outbound list entry.  A valid tag is below
+ * tag_pool.size, which is a u16, so the largest u16 is never one.
+ */
+#define MVUMI_TAG_CONSUMED		0xffff
+
 #define DEVICE_OFFLINE	0
 #define DEVICE_ONLINE	1
 
-- 
2.43.0