Re: [PATCH v2 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:

New issues:
- [High] The patch introduces a newly triggerable NULL pointer dereference if a spurious interrupt fires on an IRQ vector associated with a queue that was downgraded or freed.

Pre-existing issues:
- [High] The `out_failed` error path leaves dangling pointers in `mrioc->intr_info[*].op_reply_q`, leading to a Use-After-Free in the interrupt handler.
--

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

This commit fixes a memory leak where partially allocated DMA memory segments
were not freed if operational queue creation failed. It also hardens the
error handling path by checking for NULL pointers before freeing segments
and resetting operational queue counts to prevent deferred panics.

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

> diff --git a/drivers/scsi/mpi3mr/mpi3mr_fw.c b/drivers/scsi/mpi3mr/mpi3mr_fw.c
> --- a/drivers/scsi/mpi3mr/mpi3mr_fw.c
> +++ b/drivers/scsi/mpi3mr/mpi3mr_fw.c
[ ... ]
> @@ -2533,6 +2533,13 @@ static int mpi3mr_create_op_queues(struct mpi3mr_ioc *mrioc)
>  		}
>  	}
>  
> +	if (i < num_queues) {
> +		for (j = i; j < num_queues; j++) {
> +			mpi3mr_free_op_req_q_segments(mrioc, j);
> +			mpi3mr_free_op_reply_q_segments(mrioc, j);
> +		}
> +	}
> +

[Severity: High]
Does this introduce a NULL pointer dereference if a spurious interrupt fires
on an IRQ vector associated with a downgraded queue?

If operational queue creation partially fails during a controller reset
(e.g., i < num_queues), this loop explicitly sets 
op_reply_qinfo[j].q_segments = NULL via mpi3mr_free_op_reply_q_segments(). 
Since i > 0, the function succeeds and the controller downgrades to fewer 
queues.

However, the driver appears to leave mrioc->intr_info[j].op_reply_q pointing 
to these unreached queues. If a spurious interrupt triggers on one of the 
unreached vectors, the handler would do this:

mpi3mr_isr_primary() {
    if (intr_info->op_reply_q)
        mpi3mr_process_op_reply_q(mrioc, intr_info->op_reply_q)
}

And inside mpi3mr_process_op_reply_q():

mpi3mr_process_op_reply_q() {
    struct segments *segments = op_reply_q->q_segments;
    ...
    reply_desc = (struct mpi3_default_reply_descriptor *)segments[segment_ix].segment + ...
}

Could this unconditionally dereference the newly nullified q_segments array?

>  	if (i == 0) {
>  		/* Not even one queue is created successfully*/
>  		retval = -1;
[ ... ]
>  out_failed:
> -	kfree(mrioc->req_qinfo);
> -	mrioc->req_qinfo = NULL;
> -
> +	if (mrioc->req_qinfo) {
> +		for (j = 0; j < i; j++) {
> +			mpi3mr_free_op_req_q_segments(mrioc, j);
> +			mpi3mr_free_op_reply_q_segments(mrioc, j);
> +		}
> +		kfree(mrioc->req_qinfo);
> +		mrioc->req_qinfo = NULL;
> +	}
> +	mrioc->num_op_req_q = 0;
>  	kfree(mrioc->op_reply_qinfo);
>  	mrioc->op_reply_qinfo = NULL;
> +	mrioc->num_op_reply_q = 0;
>  
>  	return retval;
>  }

[Severity: High]
This is a pre-existing issue, but does this error path leave dangling pointers
that could cause a Use-After-Free?

If mpi3mr_create_op_queues completely fails during a controller reset 
(e.g., i == 0), the code jumps here and frees the mrioc->op_reply_qinfo array.

However, it seems to leave pointers to this array intact inside 
mrioc->intr_info[*].op_reply_q. Because these pointers were preserved from 
the initial driver load, and interrupts are re-enabled earlier in the reset 
sequence, any hardware or spurious interrupt could trigger the primary handler:

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

Could this dereference the freed op_reply_qinfo memory? Should the references
in intr_info be cleared when this array is freed?

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