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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] scsi: mvumi: TOCTOU on DMA payload bypasses checksum and bounds
- [Medium] scsi: mvumi: missing endianness conversion for frame_length
--

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

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

This patch fixes an out-of-bounds read and potential hang in the mvumi
driver by validating the frame_length from the handshake page header. It
ensures the length fits within the handshake buffer before calculating the
checksum, and uses READ_ONCE() to prevent the device from changing the
value mid-validation.

> diff --git a/drivers/scsi/mvumi.c b/drivers/scsi/mvumi.c
> index 38a92f8c25f5b..2378b676a7a78 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 = READ_ONCE(hs_header->frame_length);

[Severity: Medium]
Is an endianness conversion needed here? Hardware devices typically provide
data in little-endian format.

If run on a big-endian architecture, a valid small little-endian length
might be interpreted as a heavily byte-swapped value. This could inevitably
fail the newly added bounds check, preventing driver initialization entirely
on big-endian hosts.

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

[Severity: High]
Does validating the checksum over the shared DMA buffer guarantee the
payload's integrity when it is read later?

After validation, mvumi_hs_process_page() and mvumi_handshake() read fields
directly from the shared DMA buffer without using READ_ONCE() or making a
local copy:

mvumi_hs_process_page() {
    ...
    switch (hs_header->page_code) {
    case HS_PAGE_FIRM_CAP:
        hs_page1 = (struct mvumi_hs_page1 *) hs_header;
        mhba->max_io = hs_page1->max_io_support;
        ...
        mhba->ib_max_size_setting = hs_page1->cl_in_max_entry_size;
    ...
}

Could a malicious device concurrently modify these fields immediately after
the checksum is verified, bypassing the integrity check entirely?

This might allow the device to inject values that lead to undefined behavior
or severely corrupt the driver state machine.

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