Re: [PATCH v4 12/14] lpfc: Update ELS ACC logging for diagnostic troubleshooting

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] scsi: lpfc: UAF in lpfc_els_rsp_acc() error path due to concurrent RSCN flush
--

commit 47dddef465d8869f241a192661f8107a7fb86c19
Author: Nigel Kirkland <nkirkland2304@gmail.com>

    lpfc: Update ELS ACC logging for diagnostic troubleshooting
    
    Currently, there are ELS ACC routines that lack debug log messages to
    indicate when ACC frame transmission run into issues.  The generic
    lpfc_els_rsp_acc and more specific ACC routines are updated to log when
    there is an issue with transmitting the frame.  The routines are also
    updated to return different return codes when encountering various
    transmission issues and their function comment header is updated.

> diff --git a/drivers/scsi/lpfc/lpfc_els.c b/drivers/scsi/lpfc/lpfc_els.c
> index bf71b5a3e55e5..cb06dbc9edfab 100644
> --- a/drivers/scsi/lpfc/lpfc_els.c
> +++ b/drivers/scsi/lpfc/lpfc_els.c
[ ... ]
> @@ -6055,6 +6065,17 @@ lpfc_els_rsp_acc(struct lpfc_vport *vport, uint32_t flag,
>  			 ndlp->nlp_DID, ndlp->nlp_flag, ndlp->nlp_state,
>  			 ndlp->nlp_rpi, vport->fc_flag, kref_read(&ndlp->kref));
>  	return 0;
> +
> +err_out:
> +	if (oldiocb->cmd_dmabuf && oldiocb->cmd_dmabuf->virt)
> +		old_opcode = *(uint32_t *)oldiocb->cmd_dmabuf->virt;

[Severity: High]
Can this dereference a freed pointer?

In lpfc_els_rcv_rscn(), setting vport->fc_rscn_flush = 0 publishes the buffer
for concurrent processing. If a parallel thread processes and frees this buffer,
and lpfc_els_rsp_acc() takes this new error path, oldiocb->cmd_dmabuf->virt
would point to freed memory.

> +
> +	lpfc_printf_vlog(vport, KERN_INFO, LOG_ELS,
> +			 "1027 Xmit ELS ACC Unsuccessful: "
> +			 "cmd: x%x, error_code: %d "
> +			 "S_ID: x%x\n", old_opcode, err,
> +			 vport->fc_myDID);
> +	return err;
>  }
[ ... ]
> @@ -8489,14 +8570,14 @@ lpfc_els_rcv_rscn(struct lpfc_vport *vport, struct lpfc_iocbq *cmdiocb,
>  	vport->fc_rscn_id_list[vport->fc_rscn_id_cnt++] = pcmd;
>  	/* Indicate we are done walking fc_rscn_id_list on this vport */
>  	vport->fc_rscn_flush = 0;
> +	/* Send back ACC */
> +	lpfc_els_rsp_acc(vport, ELS_CMD_ACC, cmdiocb, ndlp, NULL);

[Severity: High]
Does moving this lpfc_els_rsp_acc() call introduce a use-after-free risk?

By calling lpfc_els_rsp_acc() here, cmdiocb->cmd_dmabuf has been published by
clearing vport->fc_rscn_flush, but is not yet set to NULL. A concurrent thread
could free cmdiocb->cmd_dmabuf before the error path in lpfc_els_rsp_acc()
attempts to read from it.

>  	/*
>  	 * If we zero, cmdiocb->cmd_dmabuf, the calling routine will
>  	 * not try to free it.
>  	 */
>  	cmdiocb->cmd_dmabuf = NULL;
>  	lpfc_set_disctmo(vport);
> -	/* Send back ACC */
> -	lpfc_els_rsp_acc(vport, ELS_CMD_ACC, cmdiocb, ndlp, NULL);
>  	/* send RECOVERY event for ALL nodes that match RSCN payload */
>  	lpfc_rscn_recovery_check(vport);
>  	return lpfc_els_handle_rscn(vport);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260917222015.61053-1-nkirkland2304@gmail.com?part=12