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

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

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 | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)


base-commit: 24a19b953e2a3d0ea5a8c4951a0ab8f32720c907
-- 
2.43.0

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

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

diff --git a/drivers/scsi/mvumi.c b/drivers/scsi/mvumi.c
index 38a92f8c25f5b..e85400291a141 100644
--- a/drivers/scsi/mvumi.c
+++ b/drivers/scsi/mvumi.c
@@ -1040,6 +1040,13 @@ static int mvumi_hs_process_page(struct mvumi_hba *mhba,
 	struct mvumi_hs_page1 *hs_page1;
 	unsigned char page_checksum;
 
+	if (hs_header->frame_length >
+	    HSP_MAX_SIZE - sizeof(struct mvumi_hs_header)) {
+		dev_err(&mhba->pdev->dev, "invalid handshake frame length %d\n",
+			hs_header->frame_length);
+		return -1;
+	}
+
 	page_checksum = mvumi_calculate_checksum(hs_header,
 						hs_header->frame_length);
 	if (page_checksum != hs_header->checksum) {
-- 
2.43.0

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

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] scsi: mvumi: double fetch of frame_length from DMA memory bypasses check
- [Medium] scsi: mvumi: handshake abort fails to notify hardware, causing 30s hang
- [Medium] scsi: mvumi: missing endianness conversion for frame_length
--

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

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

This patch adds a bounds check to mvumi_hs_process_page() to reject an
out-of-range handshake frame_length. This prevents an infinite loop and out of
bounds read that occurs if the firmware fills in a value of 256 or more.

Cc: stable@vger.kernel.org

> diff --git a/drivers/scsi/mvumi.c b/drivers/scsi/mvumi.c
> index 38a92f8c25f5b..e85400291a141 100644
> --- a/drivers/scsi/mvumi.c
> +++ b/drivers/scsi/mvumi.c
> @@ -1040,6 +1040,13 @@ static int mvumi_hs_process_page(struct mvumi_hba *mhba,
>  	struct mvumi_hs_page1 *hs_page1;
>  	unsigned char page_checksum;
>  
> +	if (hs_header->frame_length >
> +	    HSP_MAX_SIZE - sizeof(struct mvumi_hs_header)) {

[Severity: Medium]
Does hs_header->frame_length need an endianness conversion here?

If the device writes this data into DMA memory as little-endian, reading it as
a plain u16 without le16_to_cpu() could cause a valid small length to be
interpreted as a massive value on big-endian architectures.

Could this cause the initialization probe to fail on big-endian systems?

> +		dev_err(&mhba->pdev->dev, "invalid handshake frame length %d\n",
> +			hs_header->frame_length);
> +		return -1;

[Severity: Medium]
Does returning an error here cause the driver to hang?

When mvumi_hs_process_page() returns an error, the caller mvumi_handshake()
sets its state to HS_S_ABORT and returns -1 immediately:

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

By returning immediately, it skips the iowrite32 calls at the end of the block
that notify the hardware of the state change.

Furthermore, its caller mvumi_handshake_event() completely ignores the -1
return value:

drivers/scsi/mvumi.c:mvumi_handshake_event() {
    ...
    mvumi_handshake(mhba);
    do {
        isr_status = mhba->instancet->read_fw_status_reg(mhba);
    ...
}

Will this cause the driver to fall through into the 30-second polling loop
waiting for an interrupt that will never arrive?

> +	}
> +
>  	page_checksum = mvumi_calculate_checksum(hs_header,
>  						hs_header->frame_length);

[Severity: High]
Does this introduce a double fetch regression?

The bounds check above evaluates hs_header->frame_length, but then the length
is read a second time from shared DMA memory to pass into
mvumi_calculate_checksum().

If the device firmware is untrusted, could it race the CPU and modify
frame_length in memory to a value larger than 255 after the bounds check
passes but before this second fetch?

This would pass an out-of-bounds length into the checksum loop, causing the
unsigned char loop counter to wrap around infinitely and re-triggering the
exact hang this patch intended to fix.

>  	if (page_checksum != hs_header->checksum) {

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

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

[Yehyeong Lee]

Thanks for the review.

[High] double fetch: fixed in v2 -- the handshake frame_length is now read
once into a local that feeds both the bound check and the checksum, so the
device cannot change it in between:
  https://lore.kernel.org/linux-scsi/20261013173145.3412863-1-yhlee@isslab.korea.ac.kr/

[Medium] endianness: frame_length is a native u16; mvumi reads and writes it
without byte-swapping everywhere -- the existing mvumi_calculate_checksum()
calls and the driver's own frame_length stores -- and mvumi.h uses no __le
types.  The bound check follows that convention; converting only here would
disagree with how the field is used elsewhere.

[Medium] handshake abort: this patch already replaces the unbounded-length
infinite loop with a bounded rejection.  The remaining behaviour -- the
handshake timing out on an error return -- is the driver's existing recovery
path, shared with the checksum and page-code errors.  Making
mvumi_handshake_event() fail fast on that return would also abort the
FW_STATE_STARTING restart that mvumi_handshake() performs when the firmware
reports a non-OK status, so it is a separate change that needs the handshake
protocol semantics, not something to fold in here.