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

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

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 size_t 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 only, so that the plain assignment a few lines above, which
takes the depth as a count rather than an exponent, keeps accepting every
value a u8 can hold.  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>
---
 drivers/scsi/mvumi.c | 35 +++++++++++++++++++++++++++++++----
 1 file changed, 31 insertions(+), 4 deletions(-)

diff --git a/drivers/scsi/mvumi.c b/drivers/scsi/mvumi.c
index e70d336b4ab39..b86d954f3576b 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;
+	size_t total;
 
 	if (mhba->fw_flag & MVUMI_FW_ALLOC)
 		return 0;
 
+	/* These are firmware-supplied and size the allocations below. */
+	total = (size_t)mhba->ib_max_size * mhba->max_io +
+		(size_t)mhba->ob_max_size * mhba->max_io +
+		sizeof(struct mvumi_dyn_list_entry) * (size_t)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,14 @@ 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;
+		}
 		break;
 	default:
 		dev_err(&mhba->pdev->dev, "handshake: page code error\n");

base-commit: 24a19b953e2a3d0ea5a8c4951a0ab8f32720c907
-- 
2.43.0

Re: [PATCH] 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: size_t truncation bypasses buffer overflow check on 32-bit architectures
- [High] scsi: mvumi: Out-of-bounds access due to unvalidated list_num_io
--

commit b87e745d0db555a98674b0d478b3e35f7a31fefb
Author: Yehyeong Lee <yhlee@isslab.korea.ac.kr>
scsi: mvumi: reject handshake sizes that overflow the list allocations

This commit adds firmware handshake boundary checks to reject sizes that
could overflow integer capacities. It restricts shift counts to avoid
integer overflow and introduces a check on the total required allocation
size.

> diff --git a/drivers/scsi/mvumi.c b/drivers/scsi/mvumi.c
> index e70d336b4ab39..b86d954f3576b 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;
> +	size_t total;
>  
>  	if (mhba->fw_flag & MVUMI_FW_ALLOC)
>  		return 0;
>  
> +	/* These are firmware-supplied and size the allocations below. */
> +	total = (size_t)mhba->ib_max_size * mhba->max_io +
> +		(size_t)mhba->ob_max_size * mhba->max_io +
> +		sizeof(struct mvumi_dyn_list_entry) * (size_t)mhba->max_io +
> +		160;
> +	if (total > UINT_MAX) {

[Severity: High]
Does this truncation bypass the buffer overflow check on 32-bit architectures?

The calculation casts the factors to size_t. On 32-bit architectures,
size_t is exactly 32 bits. If the firmware provides large values, the
product can silently wrap to 0 during the arithmetic.

Because total is of type size_t, it mathematically can never exceed UINT_MAX
on 32-bit systems. The check total > UINT_MAX becomes tautologically false
and is bypassed, which allows the driver to proceed and allocate undersized
buffers using the wrapped 32-bit values.

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

[ ... ]

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

[ ... ]

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

[Severity: High]
Can this lead to an out-of-bounds access if list_num_io exceeds max_io?

The patch caps cl_inout_list_depth to 31, allowing list_num_io to be up
to 2^31, but fails to check whether list_num_io exceeds max_io.

The ib_list and ob_list arrays are explicitly sized and allocated based
on max_io in mvumi_init_data(). The array indices unconditionally increment
with each IO operation and only wrap around when they reach list_num_io.

If list_num_io > max_io, the driver can sequentially advance the index
beyond the allocated array size after processing max_io commands, causing
out-of-bounds heap reads and writes.

> +		}
>  		break;
>  	default:

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

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

[Yehyeong Lee]

Thanks -- both are addressed in v2.

- 32-bit truncation: correct.  size_t is 32 bits on 32-bit targets, so the
  products wrap and "total > UINT_MAX" can never be true there.  v2 computes
  the total in u64, which is 64-bit on every architecture.

- list_num_io vs max_io: v2 rejects a handshake list depth that is zero or
  yields more entries than max_io.  The inbound and outbound rings are
  allocated for max_io entries and the slot index wraps on list_num_io, so
  bounding list_num_io by max_io keeps the index within the rings.