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

[sashiko-bot@kernel.org]

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