[PATCH 0/2] scsi: fnic: fix NVMe/FC local port setup and teardown

[PATCH 0/2] scsi: fnic: fix NVMe/FC local port setup and teardown

[Linmao Li]

Two small fixes to the NVMe/FC transport path added in commit 5efdd5cf9281
("scsi: fnic: Add the NVMe/FC transport path"), both found by reading the
code rather than by hitting them on hardware.  They apply to
scsi/for-next, since that code is not in mainline yet.

Patch 1 initializes struct nvme_fc_port_info in nvfnic_add_lport().
dev_loss_tmo is currently left uninitialized on the stack.  The transport
does not read it for a local port, so this is hygiene rather than a
behaviour fix, and it carries no Fixes tag; nvfnic_add_tport() right below
already memsets its own copy.

Patch 2 moves the kfree() of iport->nv_tmpl out of nvfnic_nvme_unload()
and into nvfnic_local_port_delete().  The template is what the transport
stores in lport->ops, and nvme_fc_unregister_localport() only calls
->localport_delete() inline when no active remote ports are left;
otherwise the call is deferred to nvme_fc_rport_inactive_on_lport(),
which dereferences lport->ops after the unregister has returned.  Today
the driver frees the template even when the removal wait times out, which
leaves the transport with a dangling ->ops.

Note that patch 2 only narrows that one use-after-free.  The rest of the
timeout path still tears the fnic down while the transport holds
lport->private == iport, so that path may deserve a wider look; I did not
want to fold that into a fix.

Patch 2's reasoning rests on the NVMe/FC transport's lifetime rules, so
linux-nvme and the FC transport maintainers are copied for a second
opinion on that argument.

Compile-tested only (allmodconfig, W=1, drivers/scsi/fnic/ clean).  I do
not have Cisco VIC hardware, so neither patch has been tested on a live
adapter.

Linmao Li (2):
  scsi: fnic: initialize the NVMe local port info before registering
  scsi: fnic: free the NVMe port template from the delete callback

 drivers/scsi/fnic/fnic_nvme.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)


base-commit: 30733f28c0347d237ffb5333fdfab7a9a2d4ed34
-- 
2.25.1

[PATCH 1/2] scsi: fnic: initialize the NVMe local port info before registering

[Linmao Li]

nvfnic_add_lport() declares struct nvme_fc_port_info on the stack and
fills in four of its five members, leaving dev_loss_tmo holding whatever
the stack happened to contain before the call.  The structure is then
handed to nvme_fc_register_localport().

nvfnic_add_tport(), which registers the remote port a few lines further
down, memsets its own struct nvme_fc_port_info first, so only the local
port path passes uninitialized data across the transport interface.

The NVMe/FC transport documents dev_loss_tmo as "Used only on a
remoteport" and does not read it in nvme_fc_register_localport(), so
there is no behavioural change today.  Initialize the structure anyway:
the driver must not depend on which members the transport happens to
consume, and any member added to struct nvme_fc_port_info later would
silently start out as stack garbage.

Signed-off-by: Linmao Li <lilinmao@kylinos.cn>
---
 drivers/scsi/fnic/fnic_nvme.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/scsi/fnic/fnic_nvme.c b/drivers/scsi/fnic/fnic_nvme.c
index b237948dcafdc..00d9d5d439a38 100644
--- a/drivers/scsi/fnic/fnic_nvme.c
+++ b/drivers/scsi/fnic/fnic_nvme.c
@@ -2216,7 +2216,7 @@ int nvfnic_add_tport(struct fnic *fnic, struct fnic_tport_s *tport,
 
 int nvfnic_add_lport(struct fnic *fnic)
 {
-	struct nvme_fc_port_info pinfo;
+	struct nvme_fc_port_info pinfo = {};
 	struct fnic_iport_s *iport = &fnic->iport;
 	int ret = 0;
 
-- 
2.25.1

[PATCH 2/2] scsi: fnic: free the NVMe port template from the delete callback

[Linmao Li]

nvfnic_nvme_unload() calls nvme_fc_unregister_localport(), waits for the
transport to call back into nvfnic_local_port_delete(), and then frees
iport->nv_tmpl.  The wait is bounded, and when it expires the driver
only warns and frees the template anyway.

iport->nv_tmpl is the struct nvme_fc_port_template the transport keeps
in lport->ops.  nvme_fc_unregister_localport() only invokes
->localport_delete() when the local port has no active remote ports
left; otherwise the call is deferred to
nvme_fc_rport_inactive_on_lport(), which dereferences lport->ops long
after the unregister call returned.  Freeing the template on the timeout
path therefore leaves the transport with a dangling ->ops.

Tie the lifetime to the callback that marks the end of the transport's
use of the template instead of to the timeout.  nvfnic_local_port_delete()
runs from ->localport_delete(), after which nvme_fc_free_lport() no
longer touches lport->ops, so freeing there is safe in both the direct
and the deferred case.  The free is done before the completion is
signalled, because the unload path may tear the fnic down, and with it
the embedded iport, as soon as it wakes up.

Fixes: 5efdd5cf9281 ("scsi: fnic: Add the NVMe/FC transport path")
Signed-off-by: Linmao Li <lilinmao@kylinos.cn>
---
 drivers/scsi/fnic/fnic_nvme.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/scsi/fnic/fnic_nvme.c b/drivers/scsi/fnic/fnic_nvme.c
index 00d9d5d439a38..a8b7b7d40b9ab 100644
--- a/drivers/scsi/fnic/fnic_nvme.c
+++ b/drivers/scsi/fnic/fnic_nvme.c
@@ -1243,7 +1243,6 @@ void nvfnic_nvme_unload(struct fnic *fnic)
 				WARN_ON(1);
 			}
 			iport->flags &= ~FNIC_LPORT_NVME_REGISTERED;
-			kfree(iport->nv_tmpl);
 		}
 	}
 
@@ -1669,6 +1668,9 @@ void nvfnic_local_port_delete(struct nvme_fc_local_port *lport)
 	FNIC_NVME_DBG(KERN_INFO, fnic, "lport delete 0x%x\n",
 		      iport->fcid);
 
+	kfree(iport->nv_tmpl);
+	iport->nv_tmpl = NULL;
+
 	spin_lock_irqsave(&fnic->fnic_lock, flags);
 	if (fnic->nvme_lport_unreg_done)
 		complete(fnic->nvme_lport_unreg_done);
-- 
2.25.1

Re: [PATCH 2/2] scsi: fnic: free the NVMe port template from the delete callback

[sashiko-bot]

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

New issues:
- [Critical] Relocating `kfree(iport->nv_tmpl)` to the asynchronous `nvfnic_local_port_delete()` callback introduces a severe arbitrary free and memory corruption vulnerability if the unregister process times out.

Pre-existing issues:
- [Critical] The driver's bounded timeout design for NVMe FC unregistration leaves the `fnic` structure vulnerable to a Use-After-Free when deferred callbacks execute.
--

--- Patch [2]: [PATCH 2/2] scsi: fnic: free the NVMe port template from the delete callback ---
commit 99ed8cf7442e7f3171f98e582e611d65cec9862a
Author: Linmao Li <lilinmao@kylinos.cn>

scsi: fnic: free the NVMe port template from the delete callback

This commit attempts to fix a dangling pointer issue by moving the
freeing of the NVMe port template to the deferred local port delete callback,
tying its lifetime to the transport's use instead of a bounded timeout.

> diff --git a/drivers/scsi/fnic/fnic_nvme.c b/drivers/scsi/fnic/fnic_nvme.c
> index 00d9d5d439a38..a8b7b7d40b9ab 100644
> --- a/drivers/scsi/fnic/fnic_nvme.c
> +++ b/drivers/scsi/fnic/fnic_nvme.c
> @@ -1243,7 +1243,6 @@ void nvfnic_nvme_unload(struct fnic *fnic)
>  				WARN_ON(1);
>  			}
>  			iport->flags &= ~FNIC_LPORT_NVME_REGISTERED;
> -			kfree(iport->nv_tmpl);
>  		}
>  	}

[ ... ]

> @@ -1669,6 +1668,9 @@ void nvfnic_local_port_delete(struct nvme_fc_local_port *lport)
>  	FNIC_NVME_DBG(KERN_INFO, fnic, "lport delete 0x%x\n",
>  		      iport->fcid);
>  
> +	kfree(iport->nv_tmpl);

[Severity: Critical]
Does moving this kfree() here introduce an arbitrary free if the unregister
process times out?

If the bounded timeout in nvfnic_nvme_unload() expires:

nvfnic_nvme_unload()
    time_remain = wait_for_completion_timeout(fnic->nvme_lport_unreg_done,
                                              msecs_to_jiffies(time_wait));

The driver can abandon the wait and eventually free the fnic structure, which
embeds the iport. When the transport later invokes this deferred callback,
the iport pointer will point to freed memory.

Could this cause the kernel to execute an arbitrary free by passing a
corrupted iport->nv_tmpl pointer to kfree()?

> +	iport->nv_tmpl = NULL;
> +
>  	spin_lock_irqsave(&fnic->fnic_lock, flags);

[Severity: Critical]
This is a pre-existing issue, but does the bounded timeout design for NVMe FC
unregistration leave the fnic structure vulnerable to a use-after-free?

Similar to the issue above, if the wait_for_completion_timeout() in
nvfnic_nvme_unload() expires and the driver frees the fnic structure, this
callback will attempt to acquire fnic->fnic_lock on freed memory.

Since the transport can asynchronously invoke the localport_delete callback
long after the driver has given up waiting, is it safe to dereference the fnic
pointer here?

>  	if (fnic->nvme_lport_unreg_done)
>  		complete(fnic->nvme_lport_unreg_done);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260819114242.3598034-1-lilinmao@kylinos.cn?part=2

Re: [PATCH 2/2] scsi: fnic: free the NVMe port template from the delete callback

[Linmao Li]

在 2026/8/19 19:52, sashiko-bot@kernel.org 写道:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
>
> New issues:
> - [Critical] Relocating `kfree(iport->nv_tmpl)` to the asynchronous `nvfnic_local_port_delete()` callback introduces a severe arbitrary free and memory corruption vulnerability if the unregister process times out.
The bot is right.  Please drop this patch; 1/2 is independent and still
stands on its own.

If the wait times out, fnic_remove() continues and eventually frees fnic,
which also frees the embedded iport still referenced by lport->private.
A later ->localport_delete() callback would therefore read nv_tmpl from
freed memory before passing it to kfree().

The underlying problem is that the bounded wait allows fnic to be freed
while the NVMe/FC transport still holds pointers into it.  I will revisit
that teardown separately.

Sorry for the noise on 2/2.


Thanks,
Linmao

>
> Pre-existing issues:
> - [Critical] The driver's bounded timeout design for NVMe FC unregistration leaves the `fnic` structure vulnerable to a Use-After-Free when deferred callbacks execute.
> --
>
> --- Patch [2]: [PATCH 2/2] scsi: fnic: free the NVMe port template from the delete callback ---
> commit 99ed8cf7442e7f3171f98e582e611d65cec9862a
> Author: Linmao Li <lilinmao@kylinos.cn>
>
> scsi: fnic: free the NVMe port template from the delete callback
>
> This commit attempts to fix a dangling pointer issue by moving the
> freeing of the NVMe port template to the deferred local port delete callback,
> tying its lifetime to the transport's use instead of a bounded timeout.
>
>> diff --git a/drivers/scsi/fnic/fnic_nvme.c b/drivers/scsi/fnic/fnic_nvme.c
>> index 00d9d5d439a38..a8b7b7d40b9ab 100644
>> --- a/drivers/scsi/fnic/fnic_nvme.c
>> +++ b/drivers/scsi/fnic/fnic_nvme.c
>> @@ -1243,7 +1243,6 @@ void nvfnic_nvme_unload(struct fnic *fnic)
>>   				WARN_ON(1);
>>   			}
>>   			iport->flags &= ~FNIC_LPORT_NVME_REGISTERED;
>> -			kfree(iport->nv_tmpl);
>>   		}
>>   	}
> [ ... ]
>
>> @@ -1669,6 +1668,9 @@ void nvfnic_local_port_delete(struct nvme_fc_local_port *lport)
>>   	FNIC_NVME_DBG(KERN_INFO, fnic, "lport delete 0x%x\n",
>>   		      iport->fcid);
>>   
>> +	kfree(iport->nv_tmpl);
> [Severity: Critical]
> Does moving this kfree() here introduce an arbitrary free if the unregister
> process times out?
>
> If the bounded timeout in nvfnic_nvme_unload() expires:
>
> nvfnic_nvme_unload()
>      time_remain = wait_for_completion_timeout(fnic->nvme_lport_unreg_done,
>                                                msecs_to_jiffies(time_wait));
>
> The driver can abandon the wait and eventually free the fnic structure, which
> embeds the iport. When the transport later invokes this deferred callback,
> the iport pointer will point to freed memory.
>
> Could this cause the kernel to execute an arbitrary free by passing a
> corrupted iport->nv_tmpl pointer to kfree()?
>
>> +	iport->nv_tmpl = NULL;
>> +
>>   	spin_lock_irqsave(&fnic->fnic_lock, flags);
> [Severity: Critical]
> This is a pre-existing issue, but does the bounded timeout design for NVMe FC
> unregistration leave the fnic structure vulnerable to a use-after-free?
>
> Similar to the issue above, if the wait_for_completion_timeout() in
> nvfnic_nvme_unload() expires and the driver frees the fnic structure, this
> callback will attempt to acquire fnic->fnic_lock on freed memory.
>
> Since the transport can asynchronously invoke the localport_delete callback
> long after the driver has given up waiting, is it safe to dereference the fnic
> pointer here?
>
>>   	if (fnic->nvme_lport_unreg_done)
>>   		complete(fnic->nvme_lport_unreg_done);