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

[sashiko-bot@kernel.org]

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

New issues:
- [Critical] FORTIFY_SOURCE panic in nvfnic_ls_req_send due to embedding kernel API struct in on-wire frame format.
- [High] Use-After-Free and double completion race on the error path of nvfnic_ls_req_send().
- [High] Data race on fnic->tx_queue in fdls_send_fcoe_frame() due to dropped fnic_lock.

Pre-existing issues:
- [High] AB-BA Deadlock between nvfnic_ls_rsp_recv() and the timer callback.
- [High] Unsafe usage of shared struct member fnic->lock_flags in spin_lock_irqsave() causes IRQ state clobbering.
--

commit e7a906f62bc9306d2818dc39a38472594580ceda
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.

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

> diff --git a/drivers/scsi/fnic/fdls_fc.h b/drivers/scsi/fnic/fdls_fc.h
> --- a/drivers/scsi/fnic/fdls_fc.h
> +++ b/drivers/scsi/fnic/fdls_fc.h
> @@ -251,6 +254,11 @@ struct fc_std_logo {
>  	struct fc_els_logo els;
>  } __packed;
>  
> +struct fc_std_ls_req {
> +	struct fc_frame_header fchdr;
> +	struct nvmefc_ls_req ls_req;
> +};
> +

[Severity: Critical]
Does this struct incorrectly embed the host API driver structure into the
on-wire frame format?

struct nvmefc_ls_req is a host API structure containing kernel pointers and
DMA addresses, not an on-wire format. 

Because the destination pointer in nvfnic_ls_req_send() is later typed as
this kernel struct, fortify bounds checking strictly limits the copy to the
struct size (around 64 bytes). 

When sending larger NVMe LS requests, won't this cause a guaranteed fatal
buffer overflow panic (__fortify_memcpy_chk) on every attempt to send a
larger request?

> diff --git a/drivers/scsi/fnic/fnic_nvme.c b/drivers/scsi/fnic/fnic_nvme.c
> --- a/drivers/scsi/fnic/fnic_nvme.c
> +++ b/drivers/scsi/fnic/fnic_nvme.c

[ ... ]

> @@ -1290,6 +1290,132 @@ void nvfnic_ls_req_timeout(struct timer_list *t)
>  	ls_req->done(ls_req, -ETIMEDOUT);
>  }
>  

[Severity: High]
This is a pre-existing issue, but does nvfnic_ls_req_timeout() cause an
AB-BA deadlock with nvfnic_ls_rsp_recv()?

In nvfnic_ls_rsp_recv(), timer_delete_sync() is called while fnic->fnic_lock
is held.

If nvfnic_ls_req_timeout() unconditionally attempts to acquire the same
fnic->fnic_lock, won't it spin indefinitely if the timer expires
concurrently on another CPU, while timer_delete_sync() spins waiting for
the callback to finish?

[Severity: High]
This is a pre-existing issue, but does the use of the globally shared
fnic->lock_flags in spin_lock_irqsave() within nvfnic_ls_req_timeout()
cause IRQ state clobbering?

spin_lock_irqsave() expects a local stack variable to save the CPU's current
IRQ state. 

If multiple CPUs execute spin_lock_irqsave() concurrently using this shared
member, they will overwrite each other's saved interrupt state. Won't this
cause spin_unlock_irqrestore() to restore an incorrect state, leading to
system lockups?

> +int nvfnic_ls_req_send(struct nvme_fc_local_port *lport,

[ ... ]

> +	pls_req = (struct nvmefc_ls_req *) (frame + FNIC_ETH_FCOE_HDRS_OFFSET +
> +					sizeof(struct fc_frame_header));
> +	memcpy(pls_req, ls_req->rqstaddr, ls_req->rqstlen);

[Severity: Critical]
Is this memcpy execution vulnerable to a fortify bounds checking panic?

As mentioned above, pls_req is cast to the smaller struct nvmefc_ls_req
pointer. Since fortify strictly bounds the copy to the struct size, sending
larger requests (like Create Association at 116 bytes) will exceed this size
and trigger a panic.

> +	list_add_tail(&nvfnic_ls_req->list, &tport->ls_req_list);
> +	nvfnic_ls_req->state = FNIC_LS_REQ_CMD_PENDING;
> +	timeout = FNIC_LS_REQ_TMO_MSECS(ls_req->timeout);
> +	mod_timer(&nvfnic_ls_req->ls_req_timer,
> +		  round_jiffies(jiffies + msecs_to_jiffies(timeout)));
> +	spin_unlock_irqrestore(&fnic->fnic_lock, flags);
> +
> +	ret = fnic_send_fcoe_frame(iport, frame, frame_size);

[Severity: High]
Can dropping fnic_lock here lead to list corruption on fnic->tx_queue?

If fnic is in a transitional state, fnic_send_fcoe_frame() delegates to
fdls_send_fcoe_frame(), which queues the frame directly onto fnic->tx_queue
using list_add_tail(). 

Since the lock was explicitly dropped before the call, won't concurrent
accesses to fnic->tx_queue from other threads (like discovery) cause
unprotected list modifications?

> +	if (ret) {
> +		timer_delete_sync(&nvfnic_ls_req->ls_req_timer);
> +		spin_lock_irqsave(&fnic->fnic_lock, flags);
> +		list_del(&nvfnic_ls_req->list);

[Severity: High]
Does this error path trigger a use-after-free and double completion race?

When mod_timer() is called earlier, ownership of the request transfers to
the timer. If fnic_send_fcoe_frame() is delayed or preempted, the timer
may expire on another CPU. This fully executes nvfnic_ls_req_timeout()
and allows the NVMe midlayer to free the memory backing ls_req and
nvfnic_ls_req.

If fnic_send_fcoe_frame() subsequently returns an error, won't this error
path blindly access the already freed nvfnic_ls_req to call
timer_delete_sync() and list_del(), resulting in a use-after-free?

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