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

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

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.
Once MVUMI_FW_ALLOC is set, reject a handshake that reports a size larger
than the one its buffer was allocated for.

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>
---
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 | 62 ++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 57 insertions(+), 5 deletions(-)

diff --git a/drivers/scsi/mvumi.c b/drivers/scsi/mvumi.c
index e70d336b4ab39..72a1516a804bb 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,19 +1061,32 @@ 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 old_max_io = mhba->max_io;
+		unsigned int old_ib = mhba->ib_max_size;
+		unsigned int old_ob = mhba->ob_max_size;
+		unsigned int old_list = mhba->list_num_io;
+
 		hs_page1 = (struct mvumi_hs_page1 *) hs_header;
 
+		/* 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->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;
+		mhba->ib_max_size = (1U << hs_page1->cl_in_max_entry_size) << 2;
 
 		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;
+		mhba->ob_max_size = (1U << hs_page1->cl_out_max_entry_size) << 2;
 
 		dev_dbg(&mhba->pdev->dev, "FW version:%d\n",
 						hs_page1->fw_ver.ver_build);
@@ -1069,9 +1095,35 @@ static int mvumi_hs_process_page(struct mvumi_hba *mhba,
 			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;
+		if (mhba->hba_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;
+			}
+			mhba->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 (mhba->list_num_io == 0 ||
+		    mhba->list_num_io > mhba->max_io) {
+			dev_err(&mhba->pdev->dev,
+				"invalid handshake list depth: %u entries for %u commands\n",
+				mhba->list_num_io, mhba->max_io);
+			return -1;
+		}
+		/* On reset the buffers are not reallocated; reject sizes that
+		 * outgrow them.
+		 */
+		if ((mhba->fw_flag & MVUMI_FW_ALLOC) &&
+		    (mhba->max_io > old_max_io || mhba->ib_max_size > old_ib ||
+		     mhba->ob_max_size > old_ob ||
+		     mhba->list_num_io > old_list)) {
+			dev_err(&mhba->pdev->dev,
+				"handshake sizes grew across reset\n");
+			return -1;
+		}
 		break;
+	}
 	default:
 		dev_err(&mhba->pdev->dev, "handshake: page code error\n");
 		return -1;

base-commit: 24a19b953e2a3d0ea5a8c4951a0ab8f32720c907
-- 
2.43.0

Re: [PATCH v3] 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: state corruption on error path bypasses check
- [Medium] scsi: mvumi: missing reset bounds check for max_target_id causes OOB read
--

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

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

This patch introduces checks to reject invalid handshake entry sizes and
list depths from the firmware to prevent allocation overflows. It bounds
the worst-case allocation sizes and rejects handshakes that exceed them,
particularly when buffer sizes grow across a host reset.

> diff --git a/drivers/scsi/mvumi.c b/drivers/scsi/mvumi.c
> index e70d336b4ab39..72a1516a804bb 100644
> --- a/drivers/scsi/mvumi.c
> +++ b/drivers/scsi/mvumi.c

[ ... ]

> @@ -1048,19 +1061,32 @@ 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 old_max_io = mhba->max_io;
> +		unsigned int old_ib = mhba->ib_max_size;
> +		unsigned int old_ob = mhba->ob_max_size;
> +		unsigned int old_list = mhba->list_num_io;
> +
>  		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;

[Severity: Medium]
Is it safe to unconditionally update mhba->max_target_id here?

If a firmware handshake during a host reset returns a max_devices_support
value larger than the initial one, the target_map buffer is not reallocated
because MVUMI_FW_ALLOC is set. During a subsequent mvumi_flush_cache() call,
could the loop using this updated max_target_id read past the end of the
target_map allocation?

[ ... ]

> +		/* On reset the buffers are not reallocated; reject sizes that
> +		 * outgrow them.
> +		 */
> +		if ((mhba->fw_flag & MVUMI_FW_ALLOC) &&
> +		    (mhba->max_io > old_max_io || mhba->ib_max_size > old_ib ||
> +		     mhba->ob_max_size > old_ob ||
> +		     mhba->list_num_io > old_list)) {
> +			dev_err(&mhba->pdev->dev,
> +				"handshake sizes grew across reset\n");
> +			return -1;
> +		}

[Severity: High]
Does returning -1 here permanently corrupt the state in the mhba structure?

Because mhba->max_io and the other size fields were already updated with
the newly reported firmware values earlier in this block, returning
without restoring them leaves the oversized values stored in the mhba
structure. If the host reset is retried, old_max_io and the other
variables will cache these newly corrupted values as the baseline.

Would this cause the growth check to evaluate to false (since new == old),
successfully bypassing the protection entirely and allowing the
out-of-bounds array accesses the patch seeks to prevent?

>  		break;
> +	}

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