Re: [PATCH v8 3/8] scsi: mvumi: bound the hotplug event bitmap

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] scsi: mvumi: missing endianness conversion for firmware size field
--

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

scsi: mvumi: bound the hotplug event bitmap

mvumi_proc_msg() takes size from the hotplug event the firmware DMAs into
the event buffer and uses it twice: as the bit count for two
find_next_zero_bit() scans, and as the byte offset of the second bitmap.
This commit rejects a size whose bitmaps do not fit within the event buffer
to prevent the driver from reading past the allocation.

> diff --git a/drivers/scsi/mvumi.c b/drivers/scsi/mvumi.c
> index 2378b676a7a78..9a3dde53dea97 100644
> --- a/drivers/scsi/mvumi.c
> +++ b/drivers/scsi/mvumi.c
> @@ -1652,11 +1652,25 @@ static int mvumi_rescan_bus(void *data)
>  static void mvumi_proc_msg(struct mvumi_hba *mhba,
>  					struct mvumi_hotplug_event *param)
>  {
> -	u16 size = param->size;
> +	u16 size = READ_ONCE(param->size);

[Severity: High]
Does this code miss an endianness conversion for the firmware size field?

The hardware firmware provides a little-endian size, so on big-endian
architectures, a valid small size (e.g., 0x0040) is natively read as a
massive byte-swapped size (0x4000) when le16_to_cpu() isn't used.

Because of the newly introduced bounds check below, this inflated value will
now fail validation, discarding the valid hotplug event entirely and rendering
the driver unable to register attached devices.

>  	const unsigned long *ar_bitmap;
>  	const unsigned long *re_bitmap;
>  	int index;
>  
> +	/*
> +	 * The second bitmap starts size bits into the event buffer and
> +	 * find_next_zero_bit() reads it a long at a time, so that is the
> +	 * furthest the two scans reach.  Bound it by the length the firmware
> +	 * was told it may write, not by the buffer: the bytes past that are
> +	 * still zero, and a zero bit here means a device is present.
> +	 */
> +	if (sizeof(*param) + (size >> 3) + BITS_TO_LONGS(size) * sizeof(long) >
> +			sizeof(struct mvumi_event_req)) {
> +		dev_err(&mhba->pdev->dev,
> +			"invalid hotplug event bitmap size %u\n", size);
> +		return;
> +	}

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