Re: [PATCH 07/17] mpi3mr: Fix out-of-bounds sense buffer access

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

New issues:
- [Low] 64-bit division by a non-constant integer causes a `__umoddi3` link error on 32-bit platforms with 64-bit DMA addresses.

Pre-existing issues:
- [High] Missing `dma_rmb()` in the operational and admin reply queue polling loops.
--

--- Patch [7]: [PATCH 07/17] mpi3mr: Fix out-of-bounds sense buffer access ---
commit 7536a65ff98a39b2e42c7eb228f93604d2b3086e
Author: Chandrakanth Patil <[email protected]>

mpi3mr: Fix out-of-bounds sense buffer access

The sense buffer address reported on completion is turned into a
virtual address with no range check, so an address outside the pool
resolves to memory that does not belong to it.

Check that it lies within the pool and is correctly aligned.

> diff --git a/drivers/scsi/mpi3mr/mpi3mr_fw.c b/drivers/scsi/mpi3mr/mpi3mr_fw.c
> index 5e61448d8dbc6..103ce9dd22666 100644
> --- a/drivers/scsi/mpi3mr/mpi3mr_fw.c
> +++ b/drivers/scsi/mpi3mr/mpi3mr_fw.c
> @@ -141,6 +141,12 @@ void *mpi3mr_get_sensebuf_virt_addr(struct mpi3mr_ioc *mrioc,
>  	if (!phys_addr)
>  		return NULL;
>  
> +	if (phys_addr < mrioc->sense_buf_dma ||
> +	    (phys_addr > mrioc->sense_buf_dma +
> +	     (mrioc->num_sense_bufs * MPI3MR_SENSE_BUF_SZ) - MPI3MR_SENSE_BUF_SZ) ||
> +	    ((phys_addr - mrioc->sense_buf_dma) % MPI3MR_SENSE_BUF_SZ))

[Severity: Low]
Does this modulo operation cause a build failure on 32-bit platforms with
64-bit DMA addresses?

When building for a 32-bit architecture with CONFIG_ARCH_DMA_ADDR_T_64BIT
enabled (such as x86 PAE or ARM LPAE), dma_addr_t is 64-bit. Modulo
dividing a 64-bit integer by a runtime variable forces the compiler to
emit a call to __umoddi3, which is not exported in the kernel.

The issue was flagged in mpi3mr_get_reply_virt_addr() within the same file:

drivers/scsi/mpi3mr/mpi3mr_fw.c:mpi3mr_get_reply_virt_addr() {
    ...
	if ((phys_addr < mrioc->reply_buf_dma) ||
	    (phys_addr > mrioc->reply_buf_dma_max_address - mrioc->reply_sz) ||
	    ((phys_addr - mrioc->reply_buf_dma) % mrioc->reply_sz))
		return NULL;
    ...
}

[Severity: High]
This is a pre-existing issue, but does the reply queue processing miss
memory barriers?

Looking at the polling loops for both the operational and admin reply queues,
the phase bit is checked without a subsequent dma_rmb():

drivers/scsi/mpi3mr/mpi3mr_fw.c:mpi3mr_process_op_reply_q() {
    ...
	if ((le16_to_cpu(reply_desc->reply_flags) &
	    MPI3_REPLY_DESCRIPT_FLAGS_PHASE_MASK) != exp_phase) {
		atomic_dec(&op_reply_q->in_use);
		return 0;
	}
    ...
}

And similarly in mpi3mr_process_admin_reply_q():

drivers/scsi/mpi3mr/mpi3mr_fw.c:mpi3mr_process_admin_reply_q() {
    ...
	if ((le16_to_cpu(reply_desc->reply_flags) &
	    MPI3_REPLY_DESCRIPT_FLAGS_PHASE_MASK) != exp_phase) {
		atomic_dec(&mrioc->admin_reply_q_in_use);
		return 0;
	}
    ...
}

Under high concurrent I/O throughput on weakly-ordered architectures like
ARM64, when the hardware DMAs a reply descriptor, could the CPU speculatively
execute reads for the descriptor payload or sense buffer before the phase bit
condition is definitively synchronized?

If the phase bit changes, without a memory barrier, subsequent reads to other
descriptor fields might bypass the phase bit evaluation and read stale memory
data from the previous iteration.

> +		return NULL;
> +
>  	return mrioc->sense_buf + (phys_addr - mrioc->sense_buf_dma);
>  }

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