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

[sashiko-bot@kernel.org]

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

New issues:
- [High] Reordering lpfc_els_rsp_acc() introduces a Use-After-Free window for the RSCN payload buffer.

Pre-existing issues:
- [Critical] Integer overflow in `lpfc_els_rsp_rpl_acc` leads to a massive heap buffer overflow.
--

--- Patch [12]: [PATCH v3 12/14] lpfc: Update ELS ACC logging for diagnostic troubleshooting ---
commit 8be36e657b7a29a5634f38457734d0cc4e29c5eb
Author: Justin Tee <justintee8345@gmail.com>

lpfc: Update ELS ACC logging for diagnostic troubleshooting

This commit adds debug log messages to ELS ACC routines in the lpfc driver
to indicate when ACC frame transmission fails. It also updates the routines
to return distinct error codes for different transmission issues and updates
the corresponding function comment headers.

> diff --git a/drivers/scsi/lpfc/lpfc_els.c b/drivers/scsi/lpfc/lpfc_els.c
> index 73552ce4bc64b..0a7e69dcb2dbe 100644
> --- a/drivers/scsi/lpfc/lpfc_els.c
> +++ b/drivers/scsi/lpfc/lpfc_els.c

[ ... ]

> @@ -6039,6 +6049,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;
> +
> +	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;
>  }

[ ... ]

> @@ -8473,14 +8554,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 this reordering introduce a race condition with concurrent freeing?

By moving lpfc_els_rsp_acc() here, it now executes after vport->fc_rscn_flush
is set to 0, but before cmdiocb->cmd_dmabuf is set to NULL.

If a concurrent thread observes fc_rscn_flush == 0, it can process the list
and free the buffer.

If lpfc_els_rsp_acc() subsequently fails, the newly added err_out path shown
above would dereference the freed oldiocb->cmd_dmabuf->virt, potentially
causing a use-after-free.

Should cmdiocb->cmd_dmabuf be set to NULL before calling lpfc_els_rsp_acc(),
or should the flush flag be cleared after?

>  	/*
>  	 * 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);

[ ... ]

> @@ -9280,7 +9361,9 @@ lpfc_send_rrq(struct lpfc_hba *phba, struct lpfc_node_rrq *rrq)
>   *
>   * Return code
>   *   0 - Successfully issued ACC RPL ELS command
> - *   1 - Failed to issue ACC RPL ELS command
> + *   -ENOMEM - IOCB not prepped successfully
> + *   -EIO - The IOCB failed to issue successfully
> + *   -ENODEV - No associated node for IOCB
>   **/
>  static int
>  lpfc_els_rsp_rpl_acc(struct lpfc_vport *vport, uint16_t cmdsize,

[Severity: Critical]
This is a pre-existing issue, but I noticed a potential integer overflow
leading to a massive heap buffer overflow in this function.

The cmdsize parameter is a uint16_t. When called from lpfc_els_rcv_rpl(),
cmdsize is calculated using an unvalidated maxsize from the remote request:

lpfc_els_rcv_rpl() {
    ...
    cmdsize = sizeof(uint32_t) + maxsize * sizeof(uint32_t);
    ...
    lpfc_els_rsp_rpl_acc(vport, cmdsize, cmdiocb, ndlp);
}

If an attacker sends an RPL request with maxsize = 16383, cmdsize evaluates
to 65536, which truncates to 0 in the uint16_t argument.

Later in lpfc_els_rsp_rpl_acc(), the following memcpy is executed:

    memcpy(pcmd, &rpl_rsp, cmdsize - sizeof(uint32_t));

Since cmdsize is 0, the subtraction underflows, resulting in an enormous
copy size. Could maxsize be validated in lpfc_els_rcv_rpl() to prevent this?

> @@ -9294,12 +9377,15 @@ lpfc_els_rsp_rpl_acc(struct lpfc_vport *vport, uint16_t cmdsize,
>  	struct lpfc_iocbq *elsiocb;
>  	uint8_t *pcmd;
>  	u32 ulp_context;
> +	int err;
>  
>  	elsiocb = lpfc_prep_els_iocb(vport, 0, cmdsize, oldiocb->retry, ndlp,
>  				     ndlp->nlp_DID, ELS_CMD_ACC);
>  
> -	if (!elsiocb)
> -		return 1;
> +	if (!elsiocb) {
> +		err = -ENOMEM;
> +		goto err_out;
> +	}

[ ... ]

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