Re: [PATCH 02/17] mpi3mr: Fix out-of-bounds read when copying BSG MPI requests

On Tue, Aug 18, 2026 at 11:45 PM <[email protected]> wrote:
>
> Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
>
> Pre-existing issues:
> - [Critical] On 32-bit architectures, pointer wrap-around and integer truncation bypass buffer bounds checks, leading to arbitrary kernel memory read/write.
> - [Critical] The driver fails to validate the user-provided `job->request_len` before casting and parsing the BSG request packet, leading to an out-of-bounds heap read and a kernel panic.
> - [High] Failure to decrement `stgt_priv->block_io` on early failure of `mpi3mr_admin_request_post`.
> - [High] DMA coherent memory allocated for PRP lists is leaked on command timeout or post failure.
> --
>
> --- Patch [2]: [PATCH 02/17] mpi3mr: Fix out-of-bounds read when copying BSG MPI requests ---
> commit 0259dbaa420364e7270c5c71574f90668291edf7
> Author: Chandrakanth Patil <[email protected]>
>
> mpi3mr: Fix out-of-bounds read when copying BSG MPI requests
>
> The MPI request is copied out of the caller supplied payload without
> first checking that the requested amount is actually present, so the
> copy can read past the end of the payload buffer.
>
> Check the range before copying.
>
> Fixes: 506bc1a0d6ba ("scsi: mpi3mr: Add support for MPT commands")
> Signed-off-by: Chandrakanth Patil <[email protected]>
>
> > diff --git a/drivers/scsi/mpi3mr/mpi3mr_app.c b/drivers/scsi/mpi3mr/mpi3mr_app.c
> > index 8e5d24793efd0..1cd1c38dcd565 100644
> > --- a/drivers/scsi/mpi3mr/mpi3mr_app.c
> > +++ b/drivers/scsi/mpi3mr/mpi3mr_app.c
> > @@ -2538,6 +2538,14 @@ static long mpi3mr_bsg_process_mpt_cmds(struct bsg_job *job)
> >                               rval = -EINVAL;
> >                               goto out;
> >                       }
> > +                     if (sgl_iter + mpi_msg_size >
> > +                         dout_buf + job->request_payload.payload_len) {
>
> [Severity: Critical]
> This is a pre-existing issue, but can pointer arithmetic wrap around on 32-bit
> architectures and bypass this new bounds check?
>
> If a userspace application sends an MPI3MR_MPT_CMD BSG request with a crafted
> buf_len (e.g., 0x10001000) such that the sum modulo 2^32 remains small, the
> sgl_iter pointer wrap-around could mathematically bypass both the end-of-buffer
> limit check and this newly added check.
>
> Concurrently, can kern_buf_len / MPI3MR_IOCTL_SGE_SIZE truncate from 0x10001
> to 1 in the u16 needed_desc variable in mpi3mr_map_data_buffer_dma()?
>
> This would bypass the descriptor limits, allowing the driver to create DMA
> mappings for arbitrary kernel addresses.
>
> > +                             dprint_bsg_err(mrioc, "%s: MPI request buf exceeds dout_buf\n",
> > +                                            __func__);
> > +                             mutex_unlock(&mrioc->bsg_cmds.mutex);
> > +                             rval = -EINVAL;
> > +                             goto out;
> > +                     }
> >                       memcpy(mpi_req, sgl_iter, mpi_msg_size);
> >                       break;
> >               default:
>
> [Severity: Critical]
> This is a pre-existing issue, but does the driver validate job->request_len
> before casting and parsing the BSG request packet?
>
> If a userspace application submits a BSG request with request_len set to 0,
> the BSG core invokes memdup_user(), which returns ZERO_SIZE_PTR (0x10).
>
> Will the driver unconditionally dereference this pointer here?
>
> drivers/scsi/mpi3mr/mpi3mr_app.c:mpi3mr_bsg_request() {
>         ...
>         switch (bsg_req->cmd_type) {
>         ...
> }
>
> Could this lead to an immediate unhandled kernel panic?  Additionally, if a
> small non-zero length is provided, does the driver read bufcnt from the
> unvalidated payload in mpi3mr_bsg_process_mpt_cmds() and loop over unallocated
> heap memory?
>
> [Severity: High]
> This is a pre-existing issue, but is there a missing decrement of
> stgt_priv->block_io on early failure of mpi3mr_admin_request_post()?
>
> When processing a SCSI task management request that specifies blocking I/O,
> stgt_priv->block_io is incremented. If mpi3mr_admin_request_post() fails,
> the code jumps directly to out_unlock:
>
> drivers/scsi/mpi3mr/mpi3mr_app.c:mpi3mr_bsg_process_mpt_cmds() {
>         ...
>                 goto out_unlock;
>         ...
> }
>
> Does this skip the atomic_dec(&stgt_priv->block_io) matching the earlier
> increment, permanently blocking all future I/O to the affected SCSI target?
>
> [Severity: High]
> This is a pre-existing issue, but is DMA coherent memory allocated for PRP
> lists leaked on command timeout or post failure?
>
> When an NVMe encapsulated request utilizes a PRP list, mpi3mr_build_nvme_prp()
> allocates a DMA coherent buffer in mrioc->prp_list_virt.
>
> If mpi3mr_admin_request_post() fails, or if wait_for_completion_timeout()
> times out, the code branches to out_unlock without freeing the memory:
>
> drivers/scsi/mpi3mr/mpi3mr_app.c:mpi3mr_bsg_process_mpt_cmds() {
>         ...
>         if (mrioc->prp_list_virt) {
>                 dma_free_coherent(&mrioc->pdev->dev, mrioc->prp_sz,
>         ...
> }
>
> Would a subsequent NVMe command blindly overwrite mrioc->prp_list_virt with a
> new allocation, permanently leaking the previous DMA buffer?
>
> --
> Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2

Thanks for the review.

New issues:
- None. This patch specifically fixes an out-of-bounds read during BSG
MPI request copy by checking that sgl_iter + mpi_msg_size does not
exceed the payload length.

Pre-existing issues:
- The block_io counter leak on post failure is already addressed by
Patch 3/17 in this series.
- The pointer wrap-around on 32-bit, job->request_len validation, and
PRP DMA memory leaks are pre-existing in the driver and will be
addressed in a separate follow-up series.