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

On Tue, Aug 18, 2026 at 11:41 PM <[email protected]> wrote:
>
> 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

Thanks for the review.

New issues:
- The 32-bit build issue is valid and will be fixed in the v2
submission. switching to do_div() for alignment checking to avoid
64-bit division issues on 32-bit platforms.

Pre-existing issues:
- The missing dma_rmb() memory barrier in reply queue processing is a
pre-existing issue in the driver and will be addressed in a separate
follow-up patch series.