Re: [PATCH v6 3/5] scsi: mvumi: bound the hotplug event bitmap

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] scsi: mvumi: unaligned memory access in find_next_zero_bit()
- [High] scsi: mvumi: unaligned bitmap access and overlap in hotplug event
--

commit 858b7c6c7bb4515208e9ef60baca2c850663e824
Author: Yehyeong Lee <yhlee@isslab.korea.ac.kr>
scsi: mvumi: bound the hotplug event bitmap

This commit bounds the hotplug event bitmap to prevent the firmware from
reading past the event buffer. It checks the hotplug event size against
the expected structure length.

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

[Severity: High]
Does this code introduce an unaligned memory access and potential overlap
between bitmaps?

Looking at struct mvumi_hotplug_event, the bitmap array sits at a 4-byte
offset:

drivers/scsi/mvumi.h:
struct mvumi_hotplug_event {
	u16 size;
	u8 dummy[2];
	u8 bitmap[];
};

Since param->bitmap sits at a 4-byte offset within the DMA buffer, casting it
to unsigned long * and passing it to find_next_zero_bit() in mvumi_proc_msg()
will cause an alignment fault on strict-alignment 64-bit architectures.

Additionally, if the firmware provides a size that is not a multiple of 8,
does the size >> 3 offset truncate the remainder?

If truncated, re_bitmap will overlap with ar_bitmap's memory, which might
cause the same bits to be scanned for both device additions and removals.
This new bounds check explicitly relies on the size >> 3 offset without
accounting for the unaligned access or overlap.

> +		dev_err(&mhba->pdev->dev,
> +			"invalid hotplug event bitmap size %u\n", size);
> +		return;
> +	}
> +
>  	if (mhba->fw_flag & MVUMI_FW_ATTACH) {
>  		index = -1;
>  		ar_bitmap = (const unsigned long *) param->bitmap;

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