Re: [PATCH] scsi: mpt3sas: Avoid freeing unallocated PCIe SGL buffers

On Sat, Aug 8, 2026 at 3:39 PM <[email protected]> wrote:
>
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
>
> Pre-existing issues:
> - [High] _base_release_memory_pools() lacks idempotency, causing fatal Use-After-Free, double-frees, and NULL pointer dereferences when invoked consecutively on error paths.
> - [High] Missing NULL check for `ioc->chain_lookup[i].chains_per_smid` during partial allocation failure cleanup causes a guaranteed kernel panic due to invalid pointer arithmetic.
> --
>
> --- Patch [1]: [PATCH] scsi: mpt3sas: Avoid freeing unallocated PCIe SGL buffers ---
> commit 74366255b6baaf468aaa8aa5ff843c58339a10f7
> Author: Chandrakanth Patil <[email protected]>
>
> scsi: mpt3sas: Avoid freeing unallocated PCIe SGL buffers
>
> _base_release_memory_pools() unconditionally frees every
> ioc->pcie_sg_lookup[] entry, including ones the setup loop never
> allocated after a partial failure, causing a "bad dma" warning on
> debug kernels or a NULL pointer dereference otherwise.
>
> > diff --git a/drivers/scsi/mpt3sas/mpt3sas_base.c b/drivers/scsi/mpt3sas/mpt3sas_base.c
> > index 791a3c5fbf442..fed7aeffec58d 100644
> > --- a/drivers/scsi/mpt3sas/mpt3sas_base.c
> > +++ b/drivers/scsi/mpt3sas/mpt3sas_base.c
> > @@ -5870,6 +5870,8 @@ _base_release_memory_pools(struct MPT3SAS_ADAPTER *ioc)
> >
> >       if (ioc->pcie_sgl_dma_pool) {
> >               for (i = 0; i < ioc->scsiio_depth; i++) {
> > +                     if (!ioc->pcie_sg_lookup[i].pcie_sgl)
> > +                             continue;
>
> [Severity: High]
> This is a pre-existing issue, but does adding the array lookup here make
> the function unsafe if called twice?
>
> If _base_release_memory_pools() is invoked a second time on an error path
> (e.g., if a subsequent operation fails after a retry in
> _base_allocate_memory_pools()), ioc->pcie_sgl_dma_pool is still non-NULL
> because it was never cleared.

No, the array is already dereferenced unconditionally by the existing
dma_pool_free() call right below, under the same
if (ioc->pcie_sgl_dma_pool) guard, so this patch doesn't add a new
dereference or change the double-invocation exposure.

The underlying idempotency issue (pcie_sgl_dma_pool never cleared
after dma_pool_destroy()) is real, but pre-existing and independent of
this patch. We'd like to fix it as a separate follow-up.

>
> During the second call, the newly added check will attempt to access
> ioc->pcie_sg_lookup[i].pcie_sgl. Since ioc->pcie_sg_lookup was set to NULL
> at the end of the first call, will this trigger a NULL pointer dereference?
>
> Additionally, variables like ioc->reply_post and ioc->config_page are
> freed earlier in this function but never set to NULL. Would it make sense to
> clear these pointers and ioc->pcie_sgl_dma_pool so the function is safely
> idempotent and avoids double-frees?
>
> >                       dma_pool_free(ioc->pcie_sgl_dma_pool,
> >                                       ioc->pcie_sg_lookup[i].pcie_sgl,
> >                                       ioc->pcie_sg_lookup[i].pcie_sgl_dma);
> >                       ioc->pcie_sg_lookup[i].pcie_sgl = NULL;
> >               }
> >               dma_pool_destroy(ioc->pcie_sgl_dma_pool);
> >       }
> >       kfree(ioc->pcie_sg_lookup);
> >       ioc->pcie_sg_lookup = NULL;
>
> [Severity: High]
> This isn't a bug introduced by this patch, but while fixing the cleanup
> logic in this function, is there a similar vulnerability for
> ioc->chain_lookup[i].chains_per_smid?
>
> If kzalloc() for ioc->chain_lookup[i].chains_per_smid partially fails in
> _base_allocate_memory_pools(), the unallocated indices are NULL.
>
> In the cleanup loop further down in this function, the code calculates the
> pointer offset:
>
>         ct = &ioc->chain_lookup[i].chains_per_smid[j];
>         if (ct && ct->chain_buffer)
>                 dma_pool_free(ioc->chain_dma_pool, ...);
>
> If chains_per_smid is NULL and j > 0, C pointer arithmetic will evaluate
> this to a non-NULL address. The subsequent check if (ct && ct->chain_buffer)
> will falsely pass, leading to an immediate page fault when trying to
> dereference memory.
>
> Should we also add a NULL check for ioc->chain_lookup[i].chains_per_smid
> before evaluating the offset?

Yes, this look genuine: chains_needed_per_io is greater than one
in essentially every real configuration, so a partial kzalloc()
failure here would hit this on the very first freed index.

We'll add the chains_per_smid NULL check, but as a separate follow-up
patch to keep this one scoped to the PCIe SGL fix.

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