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

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.

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/[email protected]?part=1