[PATCH v4] scsi: mvumi: reject handshake sizes that overflow the list allocations

[PATCH v4] 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.  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.  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.  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.  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: a controller reporting 4096 commands
with 64 KiB entries and a depth of 12 needs 513 MiB and passes unchanged.

Fixes: f0c568a478f0 ("[SCSI] mvumi: Add Marvell UMI driver")
Cc: stable@vger.kernel.org
Signed-off-by: Yehyeong Lee <yhlee@isslab.korea.ac.kr>
---
v4:
- a reset does not reallocate, so validate the reported sizes against the
  current ones and leave the size fields unchanged; only the first handshake
  assigns them.  This also means a page the driver has already rejected
  cannot be presented again to get past the check, which the retry in
  mvumi_handshake_event() allows (Sashiko).
- include max_devices_support in that check; it sizes target_map, which
  mvumi_flush_cache() reads and mvumi_sdev_configure() writes (Sashiko).

v3:
- reject a reset handshake whose sizes exceed the existing allocation,
  which is not reallocated on reset (Sashiko).

v2:
- compute the allocation total in u64 so the check also holds on 32-bit
  (Sashiko).
- reject a handshake list depth whose entry count exceeds max_io, which
  would walk the per-command ring past its allocation (Sashiko).

 drivers/scsi/mvumi.c | 87 ++++++++++++++++++++++++++++++++++++++------
 1 file changed, 75 insertions(+), 12 deletions(-)

diff --git a/drivers/scsi/mvumi.c b/drivers/scsi/mvumi.c
index e70d336b4ab39..4fc0cf819c271 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;
@@ -1048,30 +1061,80 @@ 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;
+
 		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;
+		/* Firmware-supplied shift counts: (1U << n) << 2 must fit. */
+		if (hs_page1->cl_in_max_entry_size > 29 ||
+		    hs_page1->cl_out_max_entry_size > 29) {
+			dev_err(&mhba->pdev->dev,
+				"invalid handshake entry size\n");
+			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_max_io = hs_page1->max_io_support;
+		new_ib_max_size = (1U << hs_page1->cl_in_max_entry_size) << 2;
+		new_ob_max_size = (1U << hs_page1->cl_out_max_entry_size) << 2;
+		new_max_target_id = hs_page1->max_devices_support;
+		new_capability = hs_page1->capability;
+
+		new_list_num_io = hs_page1->cl_inout_list_depth;
+		if (new_capability & HS_CAPABILITY_NEW_PAGE_IO_DEPTH_DEF) {
+			if (hs_page1->cl_inout_list_depth >= 32) {
+				dev_err(&mhba->pdev->dev,
+					"invalid handshake list depth\n");
+				return -1;
+			}
+			new_list_num_io = 1U << hs_page1->cl_inout_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 = hs_page1->cl_in_max_entry_size;
+		mhba->ib_max_size = new_ib_max_size;
+		mhba->ob_max_size_setting = hs_page1->cl_out_max_entry_size;
+		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;

base-commit: 24a19b953e2a3d0ea5a8c4951a0ab8f32720c907
-- 
2.43.0

Re: [PATCH v4] scsi: mvumi: reject handshake sizes that overflow the list allocations

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] scsi: mvumi: infinite loop in mvumi_flush_cache()
- [High] scsi: mvumi: infinite loop and OOB read due to unvalidated frame_length
--

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

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

This commit adds bounds checks for firmware-supplied parameters to prevent
overflows and out-of-bounds accesses. It rejects invalid sizes for the entry
size, list depth, and allocation size, and limits sizes across host resets.

> diff --git a/drivers/scsi/mvumi.c b/drivers/scsi/mvumi.c
> index e70d336b4ab39..4fc0cf819c271 100644
> --- a/drivers/scsi/mvumi.c
> +++ b/drivers/scsi/mvumi.c
[ ... ]
> @@ -1048,30 +1061,80 @@ static int mvumi_hs_process_page(struct mvumi_hba *mhba,
>  	}
>  
>  	switch (hs_header->page_code) {

[Severity: High]
Does this code allow an infinite loop and out-of-bounds read if the firmware
provides a frame_length >= 256?

In mvumi_hs_process_page(), hs_header->frame_length is read directly from the
firmware DMA buffer and passed to mvumi_calculate_checksum() without
validation:

mvumi_hs_process_page() {
        ...
        page_checksum = mvumi_calculate_checksum(hs_header,
                                                hs_header->frame_length);
        ...
}

Looking at mvumi_calculate_checksum(), the loop counter i is an 8-bit
unsigned char:

mvumi_calculate_checksum() {
        unsigned char i;
        ...
        for (i = 0; i < len; i++) {
                ret ^= *ptr;
                ptr++;
        }
}

If frame_length >= 256, the condition i < len will always evaluate to true
since i wraps around to 0, causing the kernel to hang and read out of
bounds until a page fault occurs.

> -	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;
>  
>  		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;
> +		/* Firmware-supplied shift counts: (1U << n) << 2 must fit. */
> +		if (hs_page1->cl_in_max_entry_size > 29 ||
> +		    hs_page1->cl_out_max_entry_size > 29) {
> +			dev_err(&mhba->pdev->dev,
> +				"invalid handshake entry size\n");
> +			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_max_io = hs_page1->max_io_support;
> +		new_ib_max_size = (1U << hs_page1->cl_in_max_entry_size) << 2;
> +		new_ob_max_size = (1U << hs_page1->cl_out_max_entry_size) << 2;
> +		new_max_target_id = hs_page1->max_devices_support;
                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

[Severity: High]
Does this assignment lead to an infinite loop in mvumi_flush_cache() if
max_devices_support is greater than 255?

The new_max_target_id is derived from max_devices_support (which can be up to
65535) and is later assigned to mhba->max_target_id.

However, in mvumi_flush_cache(), the loop counter device_id is declared
as an 8-bit unsigned char:

mvumi_flush_cache() {
        unsigned char device_id;
        ...
        for (device_id = 0; device_id < mhba->max_target_id; device_id++) {
                if (!(mhba->target_map[device_id / bitcount] &
                                (1 << (device_id % bitcount))))
                        continue;
        ...
}

If max_devices_support is greater than 255, the condition
device_id < mhba->max_target_id will always be true, locking up the kernel
in an infinite loop.

> +		new_capability = hs_page1->capability;

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

Re: [PATCH v4] scsi: mvumi: reject handshake sizes that overflow the list allocations

[Yehyeong Lee]

Please drop this patch.  It is superseded by

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

where it is 4/6, with one addition: it also rejects an entry size too small
to hold the frames the driver puts in the entry.
mvumi_internal_cmd_sgl() writes one scatter-gather element at payload[0]
whatever host->sg_tablesize says, and the max_sg subtraction in
mvumi_io_attach() wraps when ib_max_size is below
sizeof(struct mvumi_msg_frame), so the lower bound was missing from this
version.

The series also folds in the two-patch length series I posted separately
and adds three more bounds from a full pass over the driver.

Re: [PATCH v4] scsi: mvumi: reject handshake sizes that overflow the list allocations

[Yehyeong Lee]

Both are real.  Neither is fixed in this version; both are fixed in the
series that replaces it:

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

- mvumi_flush_cache(): confirmed.  device_id is an unsigned char and the
  bound, mhba->max_target_id, comes from the u16 max_devices_support, so a
  reported count of 256 or more never terminates the loop, and that loop
  runs from shutdown, suspend and remove.  Fixed by 6/6 of the series,
  "scsi: mvumi: widen the flush cache loop counter"
  <20261014151914.3632701-7-yhlee@isslab.korea.ac.kr>, which gives the
  counter the type of its bound rather than rejecting the value: the rest
  of the driver already handles counts above 255 -- mvumi_probe_devices()
  walks it with an int and host->max_id is wider too -- so rejecting would
  turn away a working configuration.  frame->device_id is a u16 and
  target_map holds round_up(max_target_id, 8) / 8 bytes, so nothing
  truncates or overruns after the widening, given the reset check 4/6 adds.
  6/6 depends on 4/6 for that reason and its commit message says so.
- mvumi_calculate_checksum(): confirmed.  Fixed by 2/6,
  "scsi: mvumi: reject an out-of-range handshake frame length"
  <20261014151914.3632701-3-yhlee@isslab.korea.ac.kr>, which rejects a
  frame_length that does not fit the handshake page before the checksum
  walks it.  HSP_MAX_SIZE is 48 and the header is 4 bytes, so a
  frame_length above 44 is turned away, well below the 256 that makes the
  counter wrap, and the same patch reads the field once with READ_ONCE() so
  the checked value is the one used.  The counter width is left alone
  there, since the input can no longer reach it.

Looking for the rest of this shape turned up three more, all in the series:
a hotplug event bitmap size that is not held inside the 512-byte event
buffer (3/6), a response frame tag that is checked on the outbound-list
frame but used from the copy made afterwards, with a > instead of >= bound
(5/6), and an inbound or outbound entry size too small to hold the frames
the driver puts in it (folded into 4/6).