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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] The driver fails to transmit an FC ABTS frame when aborting or timing out an NVMe LS request, but immediately frees the local OXID, leading to potential exchange corruption and cross-talk.
- [High] Missing bounds check on `ls_req->rqstlen` leads to a heap buffer overflow when copying the NVMe LS request payload into the fixed-size FC frame.
--

--- Patch [9]: [PATCH v6 09/13] scsi: fnic: Send NVMe LS requests through FDLS ---
commit c185a494dac754c12b5da24b26550de80039d738
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, and arms request timers.

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 697a4fdfd576f..0b05dc9698be2 100644
> --- a/drivers/scsi/fnic/fnic_nvme.c
> +++ b/drivers/scsi/fnic/fnic_nvme.c
> @@ -1337,6 +1337,129 @@ void nvfnic_ls_req_timeout(struct timer_list *t)
>  	ls_req->done(ls_req, -ETIMEDOUT);
>  }

[Severity: High]
Does this timeout handler fail to transmit an FC ABTS frame?

When an NVMe LS request times out or is aborted, the driver frees the OXID
back to the pool in nvfnic_ls_req_timeout() (and the corresponding abort
handler) but appears to skip sending an FC ABTS frame to the target to
terminate the exchange.

If the target is still processing the request, it may send a delayed response.
If the freed OXID is reused for a new request, could the target's delayed
response match the new request, misrouting the payload and causing data
corruption?

> +
> +/**
> + * nvfnic_ls_req_send - Send NVMe FC link service (LS) request
[ ... ]
> +int nvfnic_ls_req_send(struct nvme_fc_local_port *lport,
> +		  struct nvme_fc_remote_port *rport,
> +		  struct nvmefc_ls_req *ls_req)
> +{
[ ... ]
> +	frame = fdls_alloc_frame(iport);
> +	if (frame == NULL) {
> +		FNIC_NVME_DBG(KERN_ERR, fnic,
> +		     "Failed to allocate frame to send NVME LS REQ");
> +		spin_unlock_irqrestore(&fnic->fnic_lock, flags);
> +		return -ENOMEM;
> +	}
[ ... ]
> +	ls_req_payload = frame + FNIC_ETH_FCOE_HDRS_OFFSET + sizeof(*fchdr);
> +	memcpy(ls_req_payload, ls_req->rqstaddr, ls_req->rqstlen);

[Severity: High]
Does this code overflow the frame buffer?

The unchecked ls_req->rqstlen is directly used as the size parameter for
memcpy() into the fixed-size mempool buffer returned by fdls_alloc_frame().

If ls_req->rqstlen is larger than the available space in the frame, could
this cause a heap buffer overflow?

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