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

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

[sashiko-bot]

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 <chandrakanth.patil@broadcom.com>

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.

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?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260808151010.185603-1-chandrakanth.patil@broadcom.com?part=1

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

[Chandrakanth Patil]

_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.

Fixes: dbec4c9040ed ("scsi: mpt3sas: lockless command submission")
Reported-by: Laurence Oberman <loberman@redhat.com>
Signed-off-by: Chandrakanth Patil <chandrakanth.patil@broadcom.com>
---
 drivers/scsi/mpt3sas/mpt3sas_base.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/scsi/mpt3sas/mpt3sas_base.c b/drivers/scsi/mpt3sas/mpt3sas_base.c
index 79052f2accbd..11bcb8440e1c 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;
 			dma_pool_free(ioc->pcie_sgl_dma_pool,
 					ioc->pcie_sg_lookup[i].pcie_sgl,
 					ioc->pcie_sg_lookup[i].pcie_sgl_dma);
-- 
2.52.0

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

[Chandrakanth Patil]

[-- Attachment #1: Type: text/plain, Size: 4847 bytes --]

On Sat, Aug 8, 2026 at 3:39 PM <sashiko-bot@kernel.org> 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 <chandrakanth.patil@broadcom.com>
>
> 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/20260808151010.185603-1-chandrakanth.patil@broadcom.com?part=1

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 5493 bytes --]