Re: [PATCH 06/17] mpi3mr: Fix out-of-bounds reply frame access

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [Low] The newly added modulo operation `((phys_addr - mrioc->reply_buf_dma) % mrioc->reply_sz)` implicitly performs a 64-bit division, causing a build failure (linker error) on 32-bit platforms where `dma_addr_t` is 64-bit.

Pre-existing issues:
- [High] An integer overflow in the reply pool size calculation allows a malicious or compromised device to trigger a massive out-of-bounds DMA write, leading to host memory corruption.
--

--- Patch [6]: [PATCH 06/17] mpi3mr: Fix out-of-bounds reply frame access ---
commit 144f6c8866bb0bb9ede9699c897280faf1028d42
Author: Chandrakanth Patil <[email protected]>

mpi3mr: Fix out-of-bounds reply frame access

This commit tightens the validation of reply frame addresses by requiring
the address to be frame-aligned and ensuring a whole frame fits within the
pool.

> diff --git a/drivers/scsi/mpi3mr/mpi3mr_fw.c b/drivers/scsi/mpi3mr/mpi3mr_fw.c
> index 681868716ebdb..5e61448d8dbc6 100644
> --- a/drivers/scsi/mpi3mr/mpi3mr_fw.c
> +++ b/drivers/scsi/mpi3mr/mpi3mr_fw.c
> @@ -128,7 +128,8 @@ void *mpi3mr_get_reply_virt_addr(struct mpi3mr_ioc *mrioc,
>  		return NULL;
>  
>  	if ((phys_addr < mrioc->reply_buf_dma) ||
> -	    (phys_addr > mrioc->reply_buf_dma_max_address))
> +	    (phys_addr > mrioc->reply_buf_dma_max_address - mrioc->reply_sz) ||
> +	    ((phys_addr - mrioc->reply_buf_dma) % mrioc->reply_sz))

[Severity: Low]
Will this modulo operation cause a linker error on 32-bit platforms?

Since dma_addr_t can be 64-bit on 32-bit architectures (like x86 PAE), using
the modulo operator on it might implicitly perform a 64-bit division. This
typically results in an undefined reference to __umoddi3 during the build.

Could this be avoided by using explicit division macros like div64_u64_rem()
or similar?

>  		return NULL;
>  
>  	return mrioc->reply_buf + (phys_addr - mrioc->reply_buf_dma);

[Severity: High]
This is a pre-existing issue, but does the reply pool size calculation in
mpi3mr_alloc_reply_sense_bufs() suffer from a 32-bit integer overflow?

If a device provides crafted values for maximum outstanding requests and
reply frame size, this multiplication might overflow:

drivers/scsi/mpi3mr/mpi3mr_fw.c:mpi3mr_alloc_reply_sense_bufs() {
    ...
    u32 sz;
    ...
    sz = mrioc->num_reply_bufs * mrioc->reply_sz;
    ...
}

If this overflows, an undersized DMA pool would be created. Later, in
mpimr_initialize_reply_sbuf_queues(), the driver loops over the expected
number of buffers and writes out-of-bounds DMA addresses to the hardware:

drivers/scsi/mpi3mr/mpi3mr_fw.c:mpimr_initialize_reply_sbuf_queues() {
    ...
    for (i = 0, phy_addr = mrioc->reply_buf_dma;
         i < mrioc->num_reply_bufs; i++, phy_addr += mrioc->reply_sz)
        mrioc->reply_free_q[i] = cpu_to_le64(phy_addr);
    ...
}

Could this allow a malicious or compromised device to trigger out-of-bounds
DMA writes into host memory?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=6