Re: [PATCH v8 7/8] scsi: mvumi: fix the DMA address rebuilt in mvumi_delete_internal_cmd()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] scsi: mvumi: missing endianness conversion for DMA address SGL fields
- [High] scsi: mvumi: memory corruption due to trusting DMA memory for allocator parameters
--

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

scsi: mvumi: fix the DMA address rebuilt in mvumi_delete_internal_cmd()

This commit fixes a bug in mvumi_delete_internal_cmd() where a 32-bit shift
on baseaddr_h resulted in truncation of the upper 32 bits of a DMA address.
The fix uses a 64-bit cast before shifting to ensure the address is properly
reconstructed for dma_free_coherent().

Cc: stable@vger.kernel.org

> diff --git a/drivers/scsi/mvumi.c b/drivers/scsi/mvumi.c
> index 2410d2aa147d9..bd1a8c1d54aeb 100644
> --- a/drivers/scsi/mvumi.c
> +++ b/drivers/scsi/mvumi.c
> @@ -295,8 +295,8 @@ static void mvumi_delete_internal_cmd(struct mvumi_hba *mhba,
>  			m_sg = (struct mvumi_sgl *) &cmd->frame->payload[0];
>  			sgd_getsz(mhba, m_sg, size);

[Severity: High]
Does reading the size natively here cause issues on big-endian architectures?
The SGL buffer size is natively read back from a little-endian field in
device DMA memory without using le32_to_cpu().

>  
> -			phy_addr = (dma_addr_t) m_sg->baseaddr_l |
> -				(dma_addr_t) ((m_sg->baseaddr_h << 16) << 16);
> +			phy_addr = (dma_addr_t)(((u64)m_sg->baseaddr_h << 32) |
> +						m_sg->baseaddr_l);

[Severity: High]
Is it safe to construct this DMA address without endianness conversion?

These SGL fields are populated using cpu_to_le32() but are read back natively.
On big-endian architectures, reading little-endian SGL fields natively yields
corrupted values, which could cause dma_free_coherent() to unmap the wrong
address or trigger a panic.

[Severity: High]
Can a compromised PCIe device modify these fields concurrently?

cmd->frame is allocated in bidirectionally mapped DMA-coherent memory,
so an untrusted device could modify sg_counts, size, baseaddr_h, and baseaddr_l
after they have been initialized or concurrently during cleanup.

Since the driver uses these values directly as parameters for
dma_free_coherent() below, this introduces a DMA double fetch / TOCTOU
vulnerability that could allow arbitrary unmapping of memory.

>  
>  			dma_free_coherent(&mhba->pdev->dev, size, cmd->data_buf,
>  								phy_addr);

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