Re: [PATCH 6.6.y] 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:
- [High] scsi: lpfc: Use-After-Free and data race due to lockless mbox assignment
- [Medium] scsi: lpfc: memory leak of mbox and mpsave on synchronous issue failure
--

commit 7e8ff891a4886b3f83580607c0dc5ee36c6f7ce1
Author: Justin Tee <justin.tee@broadcom.com>

scsi: lpfc: Handle mailbox timeouts in lpfc_get_sfp_info

This commit adds checks for the MBX_TIMEOUT return code in lpfc_get_sfp_info,
avoiding unconditional frees of timed-out mailbox commands. The timeout is
also increased from 30 to 60 seconds to accommodate boot scripts.

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 2e9972a587810..d319df7d36137 100644
> --- a/drivers/scsi/lpfc/lpfc_els.c
> +++ b/drivers/scsi/lpfc/lpfc_els.c
> @@ -7381,7 +7384,8 @@ int lpfc_get_sfp_info_wait(struct lpfc_hba *phba,
>  
>  error:
>  	mbox->ctx_buf = mpsave;

[Severity: High]
If lpfc_sli_issue_mbox_wait() times out, it transfers ownership of the mailbox
by setting:

drivers/scsi/lpfc/lpfc_sli.c:lpfc_sli_issue_mbox_wait() {
    ...
    } else {
        retval = MBX_TIMEOUT;
        pmboxq->mbox_cmpl = lpfc_sli_def_mbox_cmpl;
    }
    ...
}

Does modifying mbox->ctx_buf locklessly here introduce a data race with the
asynchronous completion handler?

If the hardware completes the mailbox command and lpfc_sli_def_mbox_cmpl()
runs concurrently, could it read an invalid payload pointer leading to a crash
in lpfc_mbox_rsrc_cleanup(), or cause a use-after-free write on the freed
mbox struct?

> -	lpfc_mbox_rsrc_cleanup(phba, mbox, MBOX_THD_UNLOCKED);
> +	if (mbox->mbox_flag & LPFC_MBX_WAKE)
> +		lpfc_mbox_rsrc_cleanup(phba, mbox, MBOX_THD_UNLOCKED);

[Severity: Medium]
When lpfc_sli_issue_mbox_wait() returns MBX_NOT_FINISHED, the LPFC_MBX_WAKE
flag is not set since the command failed to queue.

Will skipping lpfc_mbox_rsrc_cleanup() here leak the mbox and mpsave memory
on synchronous failures, since the async completion handler will never run to
clean them up?

>  	return rc;
>

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