Re: [PATCH 01/17] mpi3mr: Fix buffer overflow in BSG passthrough request copy

On Tue, Aug 18, 2026 at 11:44 PM <[email protected]> wrote:
>
> Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
>
> Pre-existing issues:
> - [Critical] Pointer arithmetic wrap-around on 32-bit architectures allows bypassing bounds checks, leading to arbitrary kernel memory read/write via DMA.
> - [High] DMA coherent memory allocated for NVMe PRPs is leaked on BSG command timeout or submission failure.
> - [High] Target device `block_io` atomic counter is leaked if BSG command submission fails.
> - [High] Lockless TOCTOU check of `MPI3MR_CMD_COMPLETE` causes a Use-After-Free of `sense_buff_k` and data races on `reply` buffers.
> - [High] Missing validation of the BSG request length (`job->request_len`) allows a user to send an undersized request buffer, leading to out-of-bounds memory reads and kernel panics.
> --
>
> --- Patch [1]: [PATCH 01/17] mpi3mr: Fix buffer overflow in BSG passthrough request copy ---
> commit ed6513158d24e842e223917348ef598d4d161609
> Author: Chandrakanth Patil <[email protected]>
> Subject: mpi3mr: Fix buffer overflow in BSG passthrough request copy
>
> This commit fixes a buffer overflow in the BSG passthrough request copy.
> The check for an incoming BSG request size used a narrower variable than
> its source field, allowing large values to wrap and pass the check.
> The variable is widened to ensure the full size is validated properly.
>
> > diff --git a/drivers/scsi/mpi3mr/mpi3mr_app.c b/drivers/scsi/mpi3mr/mpi3mr_app.c
> > index 1353a8ff9c85d..8e5d24793efd0 100644
> > --- a/drivers/scsi/mpi3mr/mpi3mr_app.c
> > +++ b/drivers/scsi/mpi3mr/mpi3mr_app.c
> > @@ -2384,7 +2384,7 @@ static long mpi3mr_bsg_process_mpt_cmds(struct bsg_job *job)
> >       long rval = -EINVAL;
> >       struct mpi3mr_ioc *mrioc = NULL;
> >       u8 *mpi_req = NULL, *sense_buff_k = NULL;
> > -     u8 mpi_msg_size = 0;
> > +     u32 mpi_msg_size = 0;
> >       struct mpi3mr_bsg_packet *bsg_req = NULL;
> >       struct mpi3mr_bsg_mptcmd *karg;
> >       struct mpi3mr_buf_entry *buf_entries = NULL;
> > @@ -2538,7 +2538,7 @@ static long mpi3mr_bsg_process_mpt_cmds(struct bsg_job *job)
> >                               rval = -EINVAL;
> >                               goto out;
> >                       }
> > -                     memcpy(mpi_req, sgl_iter, buf_entries->buf_len);
> > +                     memcpy(mpi_req, sgl_iter, mpi_msg_size);
> >                       break;
> >               default:
> >                       invalid_be = 1;
>
> [Severity: Critical]
> This is a pre-existing issue, but can the pointer arithmetic nearby lead to
> an integer wrap-around on 32-bit architectures?
>
> When iterating over buffer entries in the same function:
>
>         case MPI3MR_BSG_BUFTYPE_DATA_OUT:
>                 sgl_iter = sgl_dout_iter;
>                 sgl_dout_iter += buf_entries->buf_len;
>
> If a user submits a BSG request with a crafted buf_len, does the u32 value wrap
> the sgl_dout_iter pointer on 32-bit systems? This wrapped pointer could then
> bypass the later bounds check:
>
>         if (sgl_dout_iter > (dout_buf + job->request_payload.payload_len)) {
>
> and allow arbitrary kernel memory access during subsequent payload processing.
>
> While reviewing this patch, I noticed a few other pre-existing issues.
>
> > drivers/scsi/mpi3mr/mpi3mr_app.c:mpi3mr_bsg_request()
> > {
> >       long rval = -EINVAL;
> >       unsigned int reply_payload_rcv_len = 0;
> >
> >       struct mpi3mr_bsg_packet *bsg_req = job->request;
> >
> >       switch (bsg_req->cmd_type) {
>
> [Severity: High]
> This isn't a bug introduced by this patch, but does this function properly
> validate job->request_len before dereferencing bsg_req->cmd_type?
>
> If userspace submits a BSG request with a request_len of zero, could this
> trigger a zero size pointer dereference and cause a kernel panic? Similarly,
> if the length is too small, would this read uninitialized heap memory?
>
> > drivers/scsi/mpi3mr/mpi3mr_app.c:mpi3mr_bsg_process_mpt_cmds()
> > {
> [ ... ]
> >       if (block_io) {
> >               tgtdev = mpi3mr_get_tgtdev_by_handle(mrioc, dev_handle);
> >               if (tgtdev && tgtdev->starget && tgtdev->starget->hostdata) {
> >                       stgt_priv = (struct mpi3mr_stgt_priv_data *)
> >                           tgtdev->starget->hostdata;
> >                       atomic_inc(&stgt_priv->block_io);
> >                       mpi3mr_tgtdev_put(tgtdev);
> >               }
> >       }
> [ ... ]
> >       rval = mpi3mr_admin_request_post(mrioc, mpi_req,
> >           MPI3MR_ADMIN_REQ_FRAME_SZ, 0);
> >
> >       if (rval) {
> >               mrioc->bsg_cmds.is_waiting = 0;
> >               dprint_bsg_err(mrioc,
> >                   "%s: posting bsg request is failed\n", __func__);
> >               rval = -EAGAIN;
> >               goto out_unlock;
> >       }
>
> [Severity: High]
> This is a pre-existing issue, but does this error path permanently leak the
> block_io atomic counter?
>
> If mpi3mr_admin_request_post() fails, the goto out_unlock bypasses the
> atomic_dec(&stgt_priv->block_io) that occurs after the completion timeout
> logic. Does this leave the SCSI target device blocked indefinitely?
>
> [Severity: High]
> Similarly, this is a pre-existing issue, but does the same goto out_unlock
> skip freeing the DMA memory allocated for NVMe PRPs?
>
> If mpi3mr_build_nvme_prp() successfully allocated DMA memory in
> mrioc->prp_list_virt earlier in this function, the jump to out_unlock skips
> the success-path cleanup block:
>
>         if (mrioc->prp_list_virt) {
>                 dma_free_coherent(&mrioc->pdev->dev, mrioc->prp_sz,
>                     mrioc->prp_list_virt, mrioc->prp_list_dma);
>
> Would subsequent commands overwrite mrioc->prp_list_virt and permanently
> leak the previous DMA coherent allocation?
>
> > drivers/scsi/mpi3mr/mpi3mr_fw.c:mpi3mr_process_admin_reply_desc()
> > {
> [ ... ]
> >       cmdptr = mpi3mr_get_drv_cmd(mrioc, host_tag, def_reply);
> >       if (cmdptr) {
> >               if (cmdptr->state & MPI3MR_CMD_PENDING) {
> >                       cmdptr->state |= MPI3MR_CMD_COMPLETE;
> >                       cmdptr->ioc_loginfo = ioc_loginfo;
> [ ... ]
> >                       if (sense_buf && cmdptr->sensebuf) {
> >                               cmdptr->is_sense = 1;
> >                               memcpy(cmdptr->sensebuf, sense_buf,
> >                                      MPI3MR_SENSE_BUF_SZ);
> >                       }
>
> [Severity: High]
> This isn't a bug introduced by this patch, but does setting the
> MPI3MR_CMD_COMPLETE flag before the memory copy create a race condition?
>
> If the application thread in mpi3mr_bsg_process_mpt_cmds() wakes up from a
> natural timeout exactly when this flag is set, it performs a lockless check:
>
>         if (!(mrioc->bsg_cmds.state & MPI3MR_CMD_COMPLETE)) {
>
> Could the application thread assume the handler has fully finished, proceed
> to read uninitialized reply memory, and free sense_buff_k before the interrupt
> handler finishes copying into it? Would this result in a use-after-free when
> the interrupt handler eventually executes the memcpy?
>
> --
> Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1

Thanks for the review.

New issues:
- None. This patch specifically fixes a buffer overflow during BSG MPT
command copy by widening mpi_msg_size to u32.

Pre-existing issues:
- Issue #3 (block_io counter leak on post failure) is already
addressed by Patch 3/17 in this series.
- Issues #1, #2, #4, and #5 are pre-existing in the driver and will be
addressed in a separate follow-up series.