Re: [PATCH v2 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:

Pre-existing issues:
- [High] Out-of-bounds reply_dma addresses are reposted back to the firmware in the admin queue, allowing potential memory corruption.
- [High] Missing `dma_rmb()` after polling the DMA completion queue phase bit allows the CPU to read stale descriptor and sense data on weakly-ordered architectures.
--

commit 9016299b1f02eb19e9cbac5395a3c1dc0ee4a8cc
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 82fadb4d2b21f..5c2547bb67a52 100644
> --- a/drivers/scsi/mpi3mr/mpi3mr_fw.c
> +++ b/drivers/scsi/mpi3mr/mpi3mr_fw.c
> @@ -142,9 +142,19 @@ void *mpi3mr_get_reply_virt_addr(struct mpi3mr_ioc *mrioc,
>  void *mpi3mr_get_sensebuf_virt_addr(struct mpi3mr_ioc *mrioc,
>  	dma_addr_t phys_addr)
>  {
> +	u64 offset;
> +
>  	if (!phys_addr)
>  		return NULL;
>  
> +	offset = phys_addr - mrioc->sense_buf_dma;
> +
> +	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) ||
> +	    do_div(offset, MPI3MR_SENSE_BUF_SZ))
> +		return NULL;
> +
>  	return mrioc->sense_buf + (phys_addr - mrioc->sense_buf_dma);
>  }

While reviewing this patch, I noticed some pre-existing issues in the driver:

> static void mpi3mr_process_admin_reply_desc(struct mpi3mr_ioc *mrioc,
> 	struct mpi3_default_reply_descriptor *reply_desc, u64 *reply_dma)
> {
[ ... ]
> 	case MPI3_REPLY_DESCRIPT_FLAGS_TYPE_ADDRESS_REPLY:
> 		addr_desc = (struct mpi3_address_reply_descriptor *)reply_desc;
> 		*reply_dma = le64_to_cpu(addr_desc->reply_frame_address);
> 		def_reply = mpi3mr_get_reply_virt_addr(mrioc, *reply_dma);
> 		if (!def_reply)
> 			goto out;

[Severity: High]
This is a pre-existing issue, but does mpi3mr_process_admin_reply_desc()
leave *reply_dma populated with an invalid physical address when
mpi3mr_get_reply_virt_addr() returns NULL?

If def_reply is NULL, the code jumps to out without clearing *reply_dma.

Then in mpi3mr_process_admin_reply_q():

> 		mpi3mr_process_admin_reply_desc(mrioc, reply_desc, &reply_dma);
> 		if (reply_dma)
> 			mpi3mr_repost_reply_buf(mrioc, reply_dma);

Since *reply_dma was not cleared on error, doesn't this result in the
unvalidated or out-of-bounds address being blindly reposted to the hardware
via mpi3mr_repost_reply_buf()?

This could poison the hardware's reply free queue with arbitrary host
addresses, potentially allowing memory corruption during future DMA writes.


I also noticed another pre-existing issue in the polling logic:

> static int mpi3mr_process_admin_reply_q(struct mpi3mr_ioc *mrioc)
> {
[ ... ]
> 	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;
> 	}

[Severity: High]
This isn't a bug introduced by this patch, but is a dma_rmb() missing after
polling the DMA completion queue phase bit?

Without an address dependency between the phase bit and the rest of the
descriptor (since the pointer is already known), weakly-ordered CPU
architectures like ARM64 and PowerPC might speculatively fetch the rest of the
descriptor or the sense data before the phase bit read has actually completed.

The same pattern appears in 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 when the sense data is later read in mpi3mr_process_op_reply_desc() in
drivers/scsi/mpi3mr/mpi3mr_os.c:

> 		if (sense_buf) {
> 			scsi_normalize_sense(sense_buf, sense_count, &sshdr);

Could this result in the CPU reading stale descriptor or sense data? A
dma_rmb() seems required immediately after observing the phase bit toggle
across both the admin and operational completion paths.

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