[PATCH v2 0/2] scsi: mvumi: bound two firmware-supplied lengths

[PATCH v2 0/2] scsi: mvumi: bound two firmware-supplied lengths

[Yehyeong Lee]

Two places in mvumi trust length/count fields the Marvell UMI firmware
supplies and never check them against the buffers they index.  An event
with the maximum param_count/sense_data_length makes mvumi_show_event()
read past the 512-byte event buffer and into the log; a handshake page
with frame_length >= 256 makes the checksum loop, which uses an unsigned
char counter, spin forever during probe (shorter over-long values read
past the page).  Both date to the driver's introduction.

v2: read the handshake frame_length once into a local and use it for both
    the bound check and the checksum, so an untrusted device cannot change
    it between the two (addresses the double-fetch noted in review);
    1/2 unchanged.

Yehyeong Lee (2):
  scsi: mvumi: bound the per-event parameter and sense dumps
  scsi: mvumi: reject an out-of-range handshake frame length

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


base-commit: 24a19b953e2a3d0ea5a8c4951a0ab8f32720c907
-- 
2.43.0

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

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..910505ef4f3ee 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 = 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 v2 2/2] scsi: mvumi: reject an out-of-range handshake frame length

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] scsi: mvumi: Missing READ_ONCE() leads to double-fetch TOCTOU
- [High] scsi: mvumi: integer overflow in mvumi_init_data() allocation size
- [Medium] scsi: mvumi: dead store in handshake abort leads to 30s timeout
- [Medium] scsi: mvumi: missing endian conversion for DMA handshake fields
--

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

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

The patch attempts to fix a vulnerability in mvumi_hs_process_page() where
frame_length from the page header is used without checking it against the
HSP_MAX_SIZE buffer. It adds a bounds check to reject a frame_length that
does not fit the handshake buffer before using it. It reads frame_length once
into a local variable and uses it for both the bound check and the checksum.

Fixes: f0c568a478f0 ("[SCSI] mvumi: Add Marvell UMI driver")
Cc: stable@vger.kernel.org
Signed-off-by: Yehyeong Lee <yhlee@isslab.korea.ac.kr>

> diff --git a/drivers/scsi/mvumi.c b/drivers/scsi/mvumi.c
> index 38a92f8c25f5b..910505ef4f3ee 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 = hs_header->frame_length;

[Severity: High]
Is a compiler barrier like READ_ONCE() required for this DMA memory read?

Without it, the compiler is free to optimize away the local variable and emit
two distinct memory fetches directly from the DMA region (one for the bounds
check, one for mvumi_calculate_checksum()).

If a malicious or misbehaving device changes the memory value between the
bounds check and the checksum calculation, the validation could be bypassed.

Could the re-fetched value of >= 256 cause an infinite loop and kernel hang
inside mvumi_calculate_checksum() because its unsigned char loop counter will
indefinitely wrap around?

[Severity: Medium]
Could the unconverted frame_length from DMA memory need to be byte-swapped?

Since the firmware populates the handshake page in little-endian format, it
seems this newly added bounds check uses a byte-swapped raw value. Might this
break initialization entirely on big-endian architectures by triggering a
false positive failure?

This also appears to be an issue when the device capabilities are assigned:

drivers/scsi/mvumi.c:mvumi_hs_process_page() {
    ...
		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;
    ...
}

[Severity: High]
Are other device inputs from the handshake page validated along with
frame_length?

Device inputs are used in bitwise shifts without validation in
mvumi_hs_process_page():

    ...
		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->ob_max_size_setting = hs_page1->cl_out_max_entry_size;
		mhba->ob_max_size = (1 << hs_page1->cl_out_max_entry_size) << 2;
    ...

These unvalidated values are later multiplied and summed, which can overflow
the 32-bit tmp_size allocation parameter:

drivers/scsi/mvumi.c:mvumi_init_data() {
    ...
	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;

	tmp_size += 128 + mhba->ob_max_size * mhba->max_io;
    ...
}

This can cause out-of-bounds pointer usage when memory pointers are advanced
far beyond the under-allocated tmp_size chunk:

drivers/scsi/mvumi.c:mvumi_init_data() {
    ...
	v += mhba->ib_max_size * mhba->max_io;
	p += mhba->ib_max_size * mhba->max_io;
    ...
}

Would this lead to a heap buffer overflow and out-of-bounds DMA access?

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

[Severity: Medium]
If we return -1 here upon detecting an invalid frame length, won't this
trigger the error-handling path in mvumi_handshake():

drivers/scsi/mvumi.c:mvumi_handshake() {
		if (hs_state == HS_S_QUERY_PAGE) {
			if (mvumi_hs_process_page(mhba, hs_header)) {
				HS_SET_STATE(hs_fun, HS_S_ABORT);
				return -1;
			}
        ...
}

Does this modify the local state to signal an abort but return before
propagating this state to the hardware via iowrite32(), resulting in a
dead store?

Since the hardware is never notified of the abort, the driver spins for 30
seconds waiting for a handshake interrupt response that will never arrive.

> +	}
> +
> +	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/20261013173145.3412863-1-yhlee@isslab.korea.ac.kr?part=2

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

[Yehyeong Lee]

Thanks for the re-review.

I answered the endianness and handshake-abort points on the v1 thread --
briefly: frame_length is a native u16 that the driver reads and writes
without byte-swapping everywhere, and the error return matches the existing
checksum/page-code paths while this patch already replaces the infinite
loop with a bounded rejection:
  https://lore.kernel.org/linux-scsi/20261013173536.3414059-1-yhlee@isslab.korea.ac.kr/

On the two new points:

- READ_ONCE: v2 already compiles to a single load of frame_length here (the
  bound check and the checksum use the same register), so the double fetch
  is not present in the object code.  I can wrap the read in READ_ONCE() to
  guarantee that at the language level if you prefer.
- allocation sizing: you are right -- max_io and the cl_in/out entry sizes
  from the handshake page feed an unchecked multiply in mvumi_init_data()
  and can under-allocate.  That is a separate bug from the frame_length
  bound in this patch; I will address it in its own patch.