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

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

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.

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>
---
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 | 43 +++++++++++++++++++++++++++++++++++++++----
 1 file changed, 39 insertions(+), 4 deletions(-)

diff --git a/drivers/scsi/mvumi.c b/drivers/scsi/mvumi.c
index e70d336b4ab39..e9ba072220331 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;
@@ -1051,16 +1064,24 @@ static int mvumi_hs_process_page(struct mvumi_hba *mhba,
 	case HS_PAGE_FIRM_CAP:
 		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,8 +1090,22 @@ 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;
+		}
 		break;
 	default:
 		dev_err(&mhba->pdev->dev, "handshake: page code error\n");

base-commit: 24a19b953e2a3d0ea5a8c4951a0ab8f32720c907
-- 
2.43.0

Re: [PATCH v2] 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: endianness bug and OOB read via find_first_bit on 32-bit int
- [High] mvumi: firmware sizes overwrite allocated limits during host reset
--

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

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

This patch rejects entry sizes above 29 and list depths of 32 or more from the
firmware to prevent shift overflows and out-of-bounds array access. It also
calculates the total allocation size in a 64-bit variable, failing if it exceeds
UINT_MAX, to prevent 32-bit truncation before memory allocation.

Cc: stable@vger.kernel.org

> diff --git a/drivers/scsi/mvumi.c b/drivers/scsi/mvumi.c
> index e70d336b4ab39..e9ba072220331 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) {

[Severity: High]
Does this bypass the new bounds checks during a host reset?

During a host reset (mvumi_host_reset() -> mvumi_wait_for_outstanding() ->
mvumi_start()), a new firmware handshake is initiated. In
mvumi_hs_process_page(), structural limits like mhba->max_io and
mhba->ib_max_size are unconditionally overwritten with new firmware values.

When mvumi_init_data() is subsequently called, MVUMI_FW_ALLOC is already
set, causing an early return. This completely bypasses the new
total > UINT_MAX validation and does not reallocate buffers.

If the firmware supplies larger sizes during a reset, the driver will use
the larger bounds to index into the previously allocated, smaller buffers,
resulting in an out-of-bounds memory access.

While reviewing the changes to mhba->list_num_io, I noticed a related issue
in mvumi_hs_build_page():

drivers/scsi/mvumi.c:mvumi_hs_build_page() {
    ...
		if (mhba->hba_capability
			& HS_CAPABILITY_NEW_PAGE_IO_DEPTH_DEF) {
			hs_page4->ob_depth = find_first_bit((unsigned long *)
							    &mhba->list_num_io,
							    BITS_PER_LONG);
			hs_page4->ib_depth = find_first_bit((unsigned long *)
							    &mhba->list_num_io,
							    BITS_PER_LONG);
    ...
}

[Severity: High]
Does this code cause an out-of-bounds read and endianness bug on 64-bit systems?

Passing a pointer to a 32-bit unsigned int (mhba->list_num_io) cast to
unsigned long * to find_first_bit() reads 8 bytes on 64-bit architectures.

On big-endian systems, this will incorrectly scan the adjacent 32-bit member
(ib_max_size) instead of list_num_io, writing incorrect ob_depth and ib_depth
capabilities to the firmware.

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

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

[Yehyeong Lee]

Thanks -- v3 addresses the reset path; the find_first_bit() point is below.

- host reset: v3 rejects a reset handshake that reports sizes larger than the
  ones the buffers were allocated for.  mvumi_init_data() does not reallocate
  on reset, so mvumi_hs_process_page() now compares the new sizes against the
  current ones when MVUMI_FW_ALLOC is set and fails the handshake if any grew.
  The shift, list-depth and list_num_io checks already live in that function,
  so they applied on the reset path in v2 as well.
- find_first_bit(): this is in mvumi_hs_build_page(), which this patch does
  not change.  The 8-byte read on a 64-bit build covers list_num_io and the
  adjacent ib_max_size, both members of struct mvumi_hba, so it is a read of a
  neighbouring field rather than an out-of-bounds access.  It is pre-existing,
  and the only input that makes find_first_bit() scan past list_num_io's own
  bits -- list_num_io == 0 -- is rejected by the check this patch adds.