Re: [PATCH v6 05/13] scsi: fnic: Add FDLS role handling for NVMe initiators

[sashiko-bot@kernel.org]

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

New issues:
- [Critical] Skipping teardown steps for NVMe initiators in `fnic_remove` leaks all target-ports and their retry timers, leading to a Use-After-Free.
- [Critical] Target-port ADD and DEL events are queued for NVMe initiators but are silently dropped by the worker thread, preventing target registration.
- [High] NVMe target capability check incorrectly uses the FCP target mode bit, rejecting valid NVMe Discovery Controllers.
- [High] FC-NVMe PRLI request FCoE frames are truncated by 4 bytes, causing a payload length mismatch.

Pre-existing issues:
- [High] Target ports are permanently leaked as zombies if memory allocation fails in `fdls_delete_tport`.
- [High] Dropping the list lock inside a `list_for_each_entry_safe` loop creates a Use-After-Free race condition if the next element is removed.
- [High] Missing length validation on untrusted incoming FCoE ELS frames leads to out-of-bounds reads on uninitialized or stale DMA memory.
--

--- Patch [5]: [PATCH v6 05/13] scsi: fnic: Add FDLS role handling for NVMe initiators ---
commit 293db6e248353f1832c4c547c9d5f992bf5f84e6
Author: Karan Tilak Kumar <kartilak@cisco.com>

scsi: fnic: Add FDLS role handling for NVMe initiators
    
Modify FDLS registration and discovery flows to use NVMe FC-4 type,
features, PRLI service parameters, and FDMI attributes when the adapter
runs as an NVMe initiator. Limit SCSI host setup, teardown, rport reset,
and FC host notifications to FCP initiators while keeping target-port
events available to both FCP and NVMe roles.

> diff --git a/drivers/scsi/fnic/fdls_disc.c b/drivers/scsi/fnic/fdls_disc.c
> index f665460ef62c5..e03256183ac63 100644
> --- a/drivers/scsi/fnic/fdls_disc.c
> +++ b/drivers/scsi/fnic/fdls_disc.c
[ ... ]
> @@ -1264,11 +1284,14 @@ bool fdls_delete_tport(struct fnic_iport_s *iport, struct fnic_tport_s *tport)
>  		tport->timer_pending = 0;
>  	}
>  
> -	spin_unlock_irqrestore(&fnic->fnic_lock, fnic->lock_flags);
> -	fnic_rport_exch_reset(iport->fnic, tport->fcid);
> -	spin_lock_irqsave(&fnic->fnic_lock, fnic->lock_flags);
> +	if (IS_FNIC_FCP_INITIATOR(fnic)) {
> +		spin_unlock_irqrestore(&fnic->fnic_lock, fnic->lock_flags);
> +		fnic_rport_exch_reset(iport->fnic, tport->fcid);
> +		spin_lock_irqsave(&fnic->fnic_lock, fnic->lock_flags);
> +	}
>  
> -	if (tport->flags & FNIC_FDLS_SCSI_REGISTERED) {
> +	if ((tport->flags & FNIC_FDLS_SCSI_REGISTERED) ||
> +	    (tport->flags & FNIC_FDLS_NVME_REGISTERED)) {
>  		tport_del_evt =
>  			kzalloc_obj(struct fnic_tport_event_s, GFP_ATOMIC);
>  		if (!tport_del_evt) {

[Severity: High]
This isn't a bug introduced by this patch, but are target ports permanently
leaked as zombies if this memory allocation fails?

In fdls_delete_tport(), the state is changed to FDLS_TGT_STATE_OFFLINING
prior to this allocation. If kzalloc_obj() returns NULL, the function bails
out early without freeing the structure or unlinking it. Because the state is
now offline, will future calls return early and never clean it up?

[ ... ]
> @@ -1508,13 +1535,21 @@ fdls_send_tgt_prli(struct fnic_iport_s *iport, struct fnic_tport_s *tport)
>  		.fchdr = {.fh_r_ctl = FC_RCTL_ELS_REQ, .fh_type = FC_TYPE_ELS,
>  			  .fh_f_ctl = {FNIC_ELS_REQ_FCTL, 0, 0},
>  			  .fh_rx_id = cpu_to_be16(FNIC_UNASSIGNED_RXID)},
> -		.els_prli = {.prli_cmd = ELS_PRLI,
> -			     .prli_spp_len = 16,
> -			     .prli_len = cpu_to_be16(0x14)},
> -		.sp = {.spp_type = 0x08, .spp_flags = 0x0020,
> -		       .spp_params = cpu_to_be32(0xA2)}
> +		.els_prli = {.prli_cmd = ELS_PRLI},
> +		.sp = {.spp_params = cpu_to_be32(iport->service_params)}
>  	};
>  
> +	fdls_set_frame_type(fnic, &pprli->sp.spp_type);
> +	if (IS_FNIC_FCP_INITIATOR(fnic)) {
> +		pprli->els_prli.prli_spp_len = 16;
> +		pprli->els_prli.prli_len = cpu_to_be16(0x14);
> +		pprli->sp.spp_flags = FC_SPP_EST_IMG_PAIR;
> +	} else if (IS_FNIC_NVME_INITIATOR(fnic)) {
> +		/* Per FC-NVMe, Establish Image Pair must not be set for NVMe PRLI. */
> +		pprli->els_prli.prli_spp_len = 20;
> +		pprli->els_prli.prli_len = cpu_to_be16(0x18);
> +	}
> +

[Severity: High]
Will this payload length mismatch cause the target to reject the frame as
malformed?

In fdls_send_tgt_prli(), the FCoE frame_size is calculated statically at the
start of the function using sizeof(struct fc_std_els_prli), which assigns 16
bytes for the Service Parameter Page. For NVMe initiators, prli_spp_len is set
to 20 bytes here, making the required payload 24 bytes. Will this result in
the transmitted frame being 4 bytes shorter than the stated payload length?

[ ... ]
> @@ -2852,14 +2893,22 @@ fdls_process_tgt_prli_rsp(struct fnic_iport_s *iport,
>  	tport->fcp_csp = be32_to_cpu(prli_rsp->sp.spp_params);
>  	tport->retry_counter = 0;
>  
> -	if (tport->fcp_csp & FCP_SPPF_RETRY)
> -		tport->tgt_flags |= FNIC_FC_RP_FLAGS_RETRY;
> +	if (IS_FNIC_FCP_INITIATOR(fnic)) {
> +		if (tport->fcp_csp & FCP_SPPF_RETRY)
> +			tport->tgt_flags |= FNIC_FC_RP_FLAGS_RETRY;
> +	} else if (IS_FNIC_NVME_INITIATOR(fnic)) {
> +		if (tport->fcp_csp & FNIC_NVME_SP_SLER)
> +			tport->tgt_flags |= FNIC_FC_RP_FLAGS_RETRY;
> +	}
>  
>  	/* Check if the device plays Target Mode Function */
> -	if (!(tport->fcp_csp & FCP_PRLI_FUNC_TARGET)) {
> +	if ((IS_FNIC_FCP_INITIATOR(fnic) &&
> +	     !(tport->fcp_csp & FCP_PRLI_FUNC_TARGET)) ||
> +	    (IS_FNIC_NVME_INITIATOR(fnic) &&
> +	     !(tport->fcp_csp & FCP_PRLI_FUNC_TARGET))) {
>  		FNIC_FCS_DBG(KERN_INFO, fnic,
> -			 "Remote port(0x%x): no target support. Deleting it\n",
> -			 tgt_fcid);
> +				 "Remote port(0x%x): no target support. Deleting it\n",
> +				 tgt_fcid);
>  		fdls_tgt_logout(iport, tport);
>  		fdls_delete_tport(iport, tport);
>  		return;

[Severity: High]
Does this target capability check inappropriately reject valid NVMe Discovery
Controllers?

In fdls_process_tgt_prli_rsp(), validating target mode support checks
!(tport->fcp_csp & FCP_PRLI_FUNC_TARGET), which masks bit 4. The
NVMe Discovery Service uses bit 3. Will an NVMe Discovery Service
target asserting only bit 3 unconditionally trigger this rejection and
logout?

[ ... ]
> @@ -5087,14 +5143,18 @@ void fnic_fdls_link_down(struct fnic_iport_s *iport)
>  	fdls_set_state((&iport->fabric), FDLS_STATE_LINKDOWN);
>  	iport->fabric.flags = 0;
>  
> -	spin_unlock_irqrestore(&fnic->fnic_lock, fnic->lock_flags);
> -	fnic_fcpio_reset(iport->fnic);
> -	spin_lock_irqsave(&fnic->fnic_lock, fnic->lock_flags);
> -	list_for_each_entry_safe(tport, next, &iport->tport_list, links) {
> -		FNIC_FCS_DBG(KERN_INFO, fnic,
> +	if (IS_FNIC_FCP_INITIATOR(fnic) ||
> +	    IS_FNIC_NVME_INITIATOR(fnic)) {
> +		spin_unlock_irqrestore(&fnic->fnic_lock, fnic->lock_flags);
> +		fnic_fcpio_reset(iport->fnic);
> +		spin_lock_irqsave(&fnic->fnic_lock, fnic->lock_flags);
> +		list_for_each_entry_safe(tport, next, &iport->tport_list, links) {
> +			FNIC_FCS_DBG(KERN_INFO, fnic,
>  					 "removing rport: 0x%x", tport->fcid);
> -		fdls_delete_tport(iport, tport);
> +			fdls_delete_tport(iport, tport);
> +		}
>  	}

[Severity: High]
This is a pre-existing issue, but does dropping the list lock inside a
list_for_each_entry_safe() loop create a use-after-free race condition?

In fnic_fdls_link_down(), we iterate over tport_list using
list_for_each_entry_safe() while holding fnic_lock. Inside the loop,
fdls_delete_tport() is called, which temporarily drops the lock for FCP
initiators. If a concurrent thread processing an ELS frame or RSCN removes
and frees the next element in the list while the lock is dropped, will the
iterator advance into freed memory when the lock is reacquired?

[Severity: High]
This is also a pre-existing issue in this file, but does the lack of length
validation on FCoE ELS frames lead to out-of-bounds reads?

In fdls_process_rscn(), the loop limit num_ports is derived from the packet's
internal rscn_payload_len field. The driver doesn't appear to verify if this
claimed length fits within the physical size of the received network buffer.
If a malformed frame claims a large length, could this copy uninitialized
memory into local structures?

> diff --git a/drivers/scsi/fnic/fnic_fcs.c b/drivers/scsi/fnic/fnic_fcs.c
> index ca592bc3f6182..a67d1085d720b 100644
> --- a/drivers/scsi/fnic/fnic_fcs.c
> +++ b/drivers/scsi/fnic/fnic_fcs.c
[ ... ]
> @@ -1044,8 +1045,8 @@ void fnic_tport_event_handler(struct work_struct *work)
>  			FNIC_FCS_DBG(KERN_INFO, fnic,
>  						 "Add rport event");
>  			if (tport->state == FDLS_TGT_STATE_READY) {
> -				fnic_fdls_add_tport(&fnic->iport,
> -					(struct fnic_tport_s *) cur_evt->arg1, flags);
> +				if (IS_FNIC_FCP_INITIATOR(fnic))
> +					fnic_fdls_add_tport(&fnic->iport, tport, flags);
>  			} else {
>  				FNIC_FCS_DBG(KERN_INFO, fnic,

[Severity: Critical]
Are target-port ADD and DEL events silently dropped by the worker thread for
NVMe initiators?

In fnic_tport_event_handler(), TGT_EV_RPORT_ADD events are only processed if
IS_FNIC_FCP_INITIATOR(fnic) is true. Because there is no logic branch for
NVMe initiators, will these events be discarded without the target ever being
registered?

> diff --git a/drivers/scsi/fnic/fnic_main.c b/drivers/scsi/fnic/fnic_main.c
> index 5850d51b0e8f6..0d7828be244da 100644
> --- a/drivers/scsi/fnic/fnic_main.c
> +++ b/drivers/scsi/fnic/fnic_main.c
[ ... ]
> @@ -1184,7 +1188,8 @@ static void fnic_remove(struct pci_dev *pdev)
>  	 */
>  	flush_workqueue(fnic_event_queue);
>  
> -	fnic_scsi_unload(fnic);
> +	if (IS_FNIC_FCP_INITIATOR(fnic))
> +		fnic_scsi_unload(fnic);
>  
>  	if (vnic_dev_get_intr_mode(fnic->vdev) == VNIC_DEV_INTR_MODE_MSI)
>  		timer_delete_sync(&fnic->notify_timer);
> @@ -1228,8 +1233,10 @@ static void fnic_remove(struct pci_dev *pdev)
>  	pci_disable_device(pdev);
>  	pci_set_drvdata(pdev, NULL);
>  	ida_free(&fnic_ida, fnic->fnic_num);
> -	fnic_scsi_unload_cleanup(fnic);
> -	scsi_host_put(fnic->host);
> +	if (IS_FNIC_FCP_INITIATOR(fnic)) {
> +		fnic_scsi_unload_cleanup(fnic);
> +		scsi_host_put(fnic->host);
> +	}
>  	kfree(fnic);
>  }

[Severity: Critical]
Does skipping these teardown steps for NVMe initiators leak target ports and
their timers, leading to a use-after-free?

In fnic_remove(), fnic_scsi_unload() and fnic_scsi_unload_cleanup() are
restricted to FCP initiators. These functions handle cancelling timers and
freeing targets. If an NVMe initiator skips this cleanup, will retry timers
remain active and access the fnic context after it is freed by kfree(fnic)?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260724174811.5118-1-kartilak@cisco.com?part=5