Re: [PATCH 6.6.y v2 2/2] scsi: lpfc: Handle mailbox timeouts in lpfc_get_sfp_info

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Critical] scsi: lpfc: UAF and double free due to unsafe mbox read on timeout
- [High] scsi: lpfc: memory leak of mailbox resources on MBX_NOT_FINISHED
--

commit 0ed08ec0a3a90cbf8a1c312188b0a42b103960f6
Author: Justin Tee <justin.tee@broadcom.com>

scsi: lpfc: Handle mailbox timeouts in lpfc_get_sfp_info

The MBX_TIMEOUT return code is not handled in lpfc_get_sfp_info and the
routine unconditionally frees submitted mailbox commands regardless of
return status.  The issue is that for MBX_TIMEOUT cases, when firmware
returns SFP information at a later time, that same mailbox memory region
references previously freed memory in its cmpl routine.

Link: https://lore.kernel.org/r/20240628172011.25921-6-justintee8345@gmail.com

> diff --git a/drivers/scsi/lpfc/lpfc_els.c b/drivers/scsi/lpfc/lpfc_els.c
> index 8ef571498d846..aae08ad0f332b 100644
> --- a/drivers/scsi/lpfc/lpfc_els.c
> +++ b/drivers/scsi/lpfc/lpfc_els.c
> @@ -7309,13 +7309,13 @@ int lpfc_get_sfp_info_wait(struct lpfc_hba *phba,
>  	}
>  	mbox->vport = phba->pport;
>  	mbox->ctx_ndlp = (struct lpfc_rdp_context *)rdp_context;
> -
> -	rc = lpfc_sli_issue_mbox_wait(phba, mbox, 30);
> +	rc = lpfc_sli_issue_mbox_wait(phba, mbox, LPFC_MBOX_SLI4_CONFIG_TMO);
>  	if (rc == MBX_NOT_FINISHED) {
>  		rc = 1;
>  		goto error;
>  	}

[Severity: High]
If lpfc_sli_issue_mbox_wait() returns MBX_NOT_FINISHED, execution jumps to
the error path where the newly introduced check requires the LPFC_MBX_WAKE
flag to be set.

Since MBX_NOT_FINISHED implies the command was not queued, no asynchronous
completion will execute to free it, and LPFC_MBX_WAKE was not set. Does this
cause a permanent memory and DMA buffer leak?

> -
> +	if (rc == MBX_TIMEOUT)
> +		goto error;

[ ... ]

> @@ -7380,8 +7383,10 @@ int lpfc_get_sfp_info_wait(struct lpfc_hba *phba,
>  			     DMP_SFF_PAGE_A2_SIZE);
>  
>  error:
> -	mbox->ctx_buf = mpsave;
> -	lpfc_mbox_rsrc_cleanup(phba, mbox, MBOX_THD_UNLOCKED);
> +	if (mbox->mbox_flag & LPFC_MBX_WAKE) {

[Severity: Critical]
If lpfc_sli_issue_mbox_wait() times out, it assigns ownership of the mbox
structure to the asynchronous background handler lpfc_sli_def_mbox_cmpl()
before returning MBX_TIMEOUT.

Can an interrupt handler fire and completely free the mbox memory right after
the timeout lock is dropped in lpfc_sli_issue_mbox_wait()?

If so, does reading mbox->mbox_flag here result in a use-after-free, and
potentially a double free if the memory is quickly reallocated to another
context that sets the LPFC_MBX_WAKE bit?

> +		mbox->ctx_buf = mpsave;
> +		lpfc_mbox_rsrc_cleanup(phba, mbox, MBOX_THD_UNLOCKED);
> +	}
>  
>  	return rc;
>

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261002182205.11550-1-artem@trailofbits.com?part=2