Re: [PATCH v3 06/10] mpi3mr: Fix memory leak on operational queue creation failure

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [High] Race condition in interrupt pointer clearing causes Use-After-Free and TOCTOU NULL pointer dereferences.
- [High] Permanent DMA memory leak of `q_segment_list` when `kzalloc_objs` fails during queue allocation.
--

--- Patch [6]: [PATCH v3 06/10] mpi3mr: Fix memory leak on operational queue creation failure ---
commit 30cab2ccb258127e2dd2cf1a6d7a0d940a8b72fa
Author: Ranjan Kumar <ranjan.kumar@broadcom.com>

mpi3mr: Fix memory leak on operational queue creation failure

When operational queue creation fails after one or more queues have
been created, the error path frees the queue information arrays but
does not release the DMA memory segments associated with the created
queues, resulting in a memory leak. Fix this by ensuring that partially
allocated segments are freed immediately if a queue fails to create.

Closes: https://sashiko.dev/#/patchset/20260626114109.43685-1-ranjan.kumar@broadcom.com?part=6
Closes: https://sashiko.dev/#/patchset/20260708183305.244485-1-ranjan.kumar@broadcom.com?part=6

> diff --git a/drivers/scsi/mpi3mr/mpi3mr_fw.c b/drivers/scsi/mpi3mr/mpi3mr_fw.c
> index 9f7cee26ebcd3..370ad85681171 100644
> --- a/drivers/scsi/mpi3mr/mpi3mr_fw.c
> +++ b/drivers/scsi/mpi3mr/mpi3mr_fw.c
> @@ -2020,6 +2020,10 @@ static void mpi3mr_free_op_reply_q_segments(struct mpi3mr_ioc *mrioc, u16 q_idx)
>  	u16 j;
>  	int size;
>  	struct segments *segments;
> +	u16 midx = REPLY_QUEUE_IDX_TO_MSIX_IDX(q_idx, mrioc->op_reply_q_offset);
> +
> +	if (midx < mrioc->intr_info_count)
> +		mrioc->intr_info[midx].op_reply_q = NULL;

[Severity: High]
This isn't a bug introduced by this patch, but can this clear the pointer
while an interrupt is actively accessing it?

Since we clear mrioc->intr_info[midx].op_reply_q without disabling the
IRQ or calling synchronize_irq(), a hardware interrupt firing concurrently
could access the queue locklessly.

For example, in drivers/scsi/mpi3mr/mpi3mr_fw.c:mpi3mr_isr_poll():

    if (!atomic_read(&intr_info->op_reply_q->pend_ios))
        break;

The pointer is evaluated unconditionally and could be set to NULL
concurrently before being read again.

Similarly, in mpi3mr_isr_primary():

    if (intr_info->op_reply_q)
        num_op_reply = mpi3mr_process_op_reply_q(mrioc,
            intr_info->op_reply_q);

There is a race window where op_reply_q can be set to NULL after the
check, resulting in processing a queue whose segments are actively
being freed.

>  
>  	segments = mrioc->op_reply_qinfo[q_idx].q_segments;
>  	if (!segments)
>  		return;

[Severity: High]
This is a pre-existing issue, but could this early return bypass the
cleanup for q_segment_list?

In mpi3mr_alloc_op_reply_q_segments(), if the kzalloc_objs() call for
q_segments fails, it returns without cleaning up the successfully allocated
q_segment_list:

    op_reply_q->q_segments = kzalloc_objs(struct segments,
                                          op_reply_q->num_segments);
    if (!op_reply_q->q_segments)
        return -ENOMEM;

Because q_segments is NULL in this failure path, the cleanup function
mpi3mr_free_op_reply_q_segments() returns early here, bypassing the
dma_free_coherent() cleanup for q_segment_list. This could cause a
permanent DMA memory leak of q_segment_list.

Does this same vulnerability exist in mpi3mr_alloc_op_req_q_segments()
and mpi3mr_free_op_req_q_segments() where similar early returns occur?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260724102505.115136-1-ranjan.kumar@broadcom.com?part=6