Re: [PATCH v3 02/14] lpfc: Early return out of lpfc_els_abort when HBA_SETUP flag is not set

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] The lockless check of the `HBA_SETUP` flag introduced to prevent a use-after-free is insufficient and suffers from a TOCTOU race, leaving the UAF vulnerability unmitigated.
- [High] The iocb list entry is removed from the shared `txcmplq` without holding the required lock on an error path, leading to list corruption.
--

--- Patch [2]: [PATCH v3 02/14] lpfc: Early return out of lpfc_els_abort when HBA_SETUP flag is not set ---
commit 686b3cec858238b11486ee8f0c3a182653eb8d41
Author: Justin Tee <justintee8345@gmail.com>

lpfc: Early return out of lpfc_els_abort when HBA_SETUP flag is not set
    
It is possible that a dev_loss_tmo callback fires during an hba reset.
The ELS pring structure is cleared by the hba reset path and the
dev_loss_tmo callback executing lpfc_els_abort could be using a stale ELS
pring pointer.  To prevent such a condition, check if HBA_SETUP flag is set
before proceeding to use the ELS pring pointer in lpfc_els_abort. There is
no point to issue aborts when the sli port is not setup anyways.

> diff --git a/drivers/scsi/lpfc/lpfc_nportdisc.c b/drivers/scsi/lpfc/lpfc_nportdisc.c
> index 9c449055a55e6..2c8d995a45bf8 100644
> --- a/drivers/scsi/lpfc/lpfc_nportdisc.c
> +++ b/drivers/scsi/lpfc/lpfc_nportdisc.c
> @@ -227,6 +227,11 @@ lpfc_els_abort(struct lpfc_hba *phba, struct lpfc_nodelist *ndlp)
>  	struct lpfc_iocbq *iocb, *next_iocb;
>  	int retval = 0;
>  
> +	/* Exit early to prevent race with queue teardown. */
> +	if (unlikely(phba->sli_rev == LPFC_SLI_REV4 &&
> +		     !test_bit(HBA_SETUP, &phba->hba_flag)))
> +		return;
> +

[Severity: High]
This is a pre-existing issue, but does this lockless check of HBA_SETUP
still leave a time-of-check to time-of-use race window open?

A concurrent HBA reset thread can clear HBA_SETUP and destroy the work
queues:

lpfc_wq_destroy()
    kfree(wq->pring);
    wq->pring = NULL;

If this happens immediately after the lockless check in lpfc_els_abort()
but before the pointer is used, could we still fetch a pointer that is
about to be freed, and subsequently dereference it later when acquiring
pring->ring_lock?

>  	pring = lpfc_phba_elsring(phba);
>  
>  	/* In case of error recovery path, we might have a NULL pring here */

[Severity: High]
This is a pre-existing issue, but does the error path later in
lpfc_els_abort() remove the iocb from the shared txcmplq without holding
the required locks?

	spin_unlock_irq(&phba->hbalock);

	if (test_bit(FC_UNLOADING, &phba->pport->load_flag) &&
	    retval && retval != IOCB_ABORTING) {
		list_del_init(&iocb->list);

Since elements are iterated on pring->txcmplq using their list node and
added to a local abort_list using their dlist node, the iocb->list is
still linked into txcmplq.

Could modifying iocb->list here without pring->ring_lock or hbalock lead
to list corruption if the shared list is concurrently accessed?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260714011812.106753-1-justintee8345@gmail.com?part=2