Re: [PATCH v5 09/13] scsi: fnic: Send NVMe LS requests through FDLS

[sashiko-bot@kernel.org]

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

New issues:
- [Critical] A race condition between NVMe-FC LS request submission and abort paths allows mod_timer to be called on an uninitialized timer.

Pre-existing issues:
- [Critical] Using the shared struct member fnic->lock_flags for spin_lock_irqsave leads to IRQ flag corruption.
- [High] Missing synchronous timer cancellation in LS response processing leads to UAF in timer callback.
- [High] Dereferencing lsreq->private after dropping lock in nvfnic_ls_req_abort() causes UAF.
--

commit 6b90bcf4cce6c8a0c7e6c0e98d68dca9d9d15359
Author: Karan Tilak Kumar <kartilak@cisco.com>

scsi: fnic: Send NVMe LS requests through FDLS

This commit adds the FC frame wrapper for NVMe LS requests and builds LS
request frames from the NVMe-FC transport callback. It allocates OXIDs,
tracks outstanding LS requests on the target port, arms request timers,
and registers the LS request callback in the NVMe FC port template.

Closes: https://lore.kernel.org/oe-kbuild-all/202605280619.pmobiDWp-lkp@intel.com/

> diff --git a/drivers/scsi/fnic/fnic_nvme.c b/drivers/scsi/fnic/fnic_nvme.c
> index 8374464e4fcc8..16e2f0add5ce6 100644
> --- a/drivers/scsi/fnic/fnic_nvme.c
> +++ b/drivers/scsi/fnic/fnic_nvme.c
> @@ -1306,6 +1306,129 @@ void nvfnic_ls_req_timeout(struct timer_list *t)
>  	ls_req->done(ls_req, -ETIMEDOUT);
>  }

[Severity: Critical]
This is a pre-existing issue, but does nvfnic_ls_req_timeout() use the
shared struct member fnic->lock_flags for spin_lock_irqsave()?

If spin_lock_irqsave() saves the local IRQ state into a shared variable,
and multiple CPUs attempt to acquire the lock concurrently, won't they
overwrite fnic->lock_flags with their own IRQ state?

When releasing the lock, a CPU would restore the IRQ state written by
another CPU instead of its own, potentially leading to permanent IRQ
disablement and hard lockups.

[Severity: High]
This isn't a bug introduced by this patch, but is it possible for a
timeout and an LS response to race and cause a use-after-free here?

If a concurrent completion path like nvfnic_ls_rsp_recv() completes an
LS request using timer_delete() (which is not synchronous) and then calls
lsreq->done(), the NVMe FC transport will free the lsreq memory.

If nvfnic_ls_req_timeout() is executing concurrently on another CPU, won't
it acquire the lock and dereference the already freed lsreq memory?

> +
> +/**
> + * nvfnic_ls_req_send - Send NVMe FC link service (LS) request

[ ... ]

> +	if (fdls_alloc_oxid(iport, FNIC_FRAME_TYPE_NVME_LS,
> +			&nvfnic_ls_req->oxid) == FNIC_UNASSIGNED_OXID) {
> +		FNIC_FCS_DBG(KERN_INFO, fnic,
> +		     "0x%x: Failed to allocate OXID to send NVME LS REQ",
> +			 iport->fcid);
> +		mempool_free(frame, fnic->frame_pool);
> +		spin_unlock_irqrestore(&fnic->fnic_lock, flags);
> +		return -EAGAIN;
> +	}
> +
> +	timer_setup(&nvfnic_ls_req->ls_req_timer, nvfnic_ls_req_timeout,
> +		     0UL);

[Severity: Critical]
Is there a race condition here between the LS request submission and the
abort path?

In the NVMe-FC midlayer, __nvme_fc_send_ls_req() adds the LS request to
rport->ls_req_list before calling the ls_req callback (nvfnic_ls_req_send).

If nvme_fc_abort_lsops() executes asynchronously right after the list
insertion but before nvfnic_ls_req_send() initializes the timer here,
will it invoke nvfnic_ls_req_abort() on a request where lsreq->private
is still purely zero-initialized memory?

If so, nvfnic_ls_req_abort() would unconditionally call mod_timer() on an
uninitialized timer, which could lead to a NULL pointer dereference or
timer wheel corruption.

> +
> +	nvfnic_ls_req->fnic = fnic;
> +	nvfnic_ls_req->tport = tport;

[ ... ]

> @@ -1606,7 +1729,7 @@ nvme_fc_port_template nvfnic_port = {
>  	.remoteport_delete = nvfnic_remote_port_delete,
>  	.create_queue = nvfnic_create_queue,
>  	.delete_queue = NULL,
> -	.ls_req = NULL,
> +	.ls_req = nvfnic_ls_req_send,
>  	.ls_abort = nvfnic_ls_req_abort,

[Severity: High]
This is a pre-existing issue, but does dropping the lock in
nvfnic_ls_req_abort() open a window for a use-after-free?

In nvfnic_ls_req_abort(), fnic->fnic_lock is dropped to safely call
timer_delete_sync(). During this unlocked window, if a concurrent
completion path calls lsreq->done(), the transport will free the lsreq
memory.

When timer_delete_sync() returns and nvfnic_ls_req_abort() reacquires the
lock, it executes nvfnic_ls_req = lsreq->private. Does this blindly
dereference the freed lsreq structure?

>  	.fcp_io = nvfnic_fcpio_send,
>  	.fcp_abort = nvfnic_fcpio_abort,

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