Re: [PATCH v5 3/6] scsi: mvumi: bound the hotplug event bitmap

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] scsi: mvumi: unaligned access, endianness, and truncation in bitmap scan
- [High] scsi: mvumi: DMA address truncation in mvumi_delete_internal_cmd
- [Medium] scsi: mvumi: incomplete bounds check causes phantom hotplug events
--

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

scsi: mvumi: bound the hotplug event bitmap

This commit introduces bounds checking for the hotplug event bitmap in
mvumi_proc_msg to prevent scans from exceeding the allocated buffer size.
It also adds a defined macro for the event buffer size to keep the
allocation and bounds check synchronized.

> diff --git a/drivers/scsi/mvumi.c b/drivers/scsi/mvumi.c
> --- a/drivers/scsi/mvumi.c
> +++ b/drivers/scsi/mvumi.c
> @@ -1652,11 +1652,23 @@ 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]
Since param->size is supplied by the firmware, does this need an endianness
conversion, such as le16_to_cpu(), to prevent incorrect size bounds on
big-endian systems?

>  	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.
> +	 */
> +	if (sizeof(*param) + (size >> 3) + BITS_TO_LONGS(size) * sizeof(long) >
> +			MVUMI_EVENT_BUF_SIZE) {

[Severity: Medium]
The bounds check allows sizes up to MVUMI_EVENT_BUF_SIZE (512 bytes).
However, looking at the DMA transfer configuration in mvumi_get_event(),
the data_transfer_length is strictly limited to
sizeof(struct mvumi_event_req) which is 388 bytes.

Could this mismatch cause find_next_zero_bit() to scan unwritten,
zero-filled memory between 388 and 512 bytes, treating the zeroes as
device events and incorrectly triggering phantom SCSI hotplug additions?

> +		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;
>  		re_bitmap = (const unsigned long *) &param->bitmap[size >> 3];

[Severity: High]
Does casting param->bitmap, which is at a 4-byte offset in the DMA buffer,
to an unsigned long pointer risk causing an alignment fault on 64-bit
architectures with strict alignment rules?

Additionally, if the firmware provides a size that is not a multiple of 8,
does the bitwise shift size >> 3 truncate the byte offset, causing the
re_bitmap offline scan to incorrectly overlap with the ar_bitmap online
scan?

[ ... ]

> @@ -1708,7 +1720,7 @@ static int mvumi_get_event(struct mvumi_hba *mhba, unsigned char msg)
>  	struct mvumi_cmd *cmd;
>  	struct mvumi_msg_frame *frame;
>  
> -	cmd = mvumi_create_internal_cmd(mhba, 512);
> +	cmd = mvumi_create_internal_cmd(mhba, MVUMI_EVENT_BUF_SIZE);
>  	if (!cmd)
>  		return -1;

[ ... ]

>  	if (cmd->cmd_status != SAM_STAT_GOOD)
>  		dev_err(&mhba->pdev->dev, "get event failed, status=0x%x.\n",
>  							cmd->cmd_status);
>  	else
>  		mvumi_notification(mhba, cmd->frame->cdb[1], cmd->data_buf);
>  
>  	mvumi_delete_internal_cmd(mhba, cmd);

[Severity: High]
Looking at mvumi_delete_internal_cmd() called here during cleanup:

drivers/scsi/mvumi.c:mvumi_delete_internal_cmd() {
    ...
    phy_addr = (dma_addr_t) m_sg->baseaddr_l |
        (dma_addr_t) ((m_sg->baseaddr_h << 16) << 16);
    ...
}

Since baseaddr_h is a 32-bit u32, does this double shift evaluate in a
32-bit unsigned int context, causing the upper 32 bits to be zeroed on
64-bit systems before the cast to dma_addr_t?

Could this truncated physical address lead to a DMA unmap memory
corruption or resource leak when reconstructing the original address?

>  	return 0;
>  }

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