Re: [PATCH 5/5] scsi: lpfc: Replace strlcat() with sysfs_emit_at() in the sysfs show functions

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

Pre-existing issues:
- [High] Use-after-free of `nrport` because it is dereferenced outside of its protecting lock.
--

--- Patch [5]: [PATCH 5/5] scsi: lpfc: Replace strlcat() with sysfs_emit_at() in the sysfs show functions ---
commit b85263748e0a979a8db2d3fc65264e450144763e
Author: Ian Bridges <[email protected]>
scsi: lpfc: Replace strlcat() with sysfs_emit_at() in the sysfs show functions

This commit replaces strlcat() with sysfs_emit_at() in several lpfc sysfs
show functions. It also removes the intermediate tmp buffers and per-fragment
overflow checks.

Link: https://github.com/KSPP/linux/issues/370 [1]

> diff --git a/drivers/scsi/lpfc/lpfc_attr.c b/drivers/scsi/lpfc/lpfc_attr.c
> --- a/drivers/scsi/lpfc/lpfc_attr.c
> +++ b/drivers/scsi/lpfc/lpfc_attr.c

[ ... ]

> @@ -702,77 +647,55 @@ lpfc_nvme_info_show(struct device *dev, struct device_attribute *attr,
>  		}
>  
>  		/* Tab in to show lport ownership. */
> -		if (strlcat(buf, "NVME RPORT       ", PAGE_SIZE) >= PAGE_SIZE)
> -			goto unlock_buf_done;
> -		if (phba->brd_no >= 10) {
> -			if (strlcat(buf, " ", PAGE_SIZE) >= PAGE_SIZE)
> -				goto unlock_buf_done;
> -		}
> +		len += sysfs_emit_at(buf, len, "NVME RPORT       ");
> +		if (phba->brd_no >= 10)
> +			len += sysfs_emit_at(buf, len, " ");
>  
> -		scnprintf(tmp, sizeof(tmp), "WWPN x%llx ",
> -			  nrport->port_name);
> -		if (strlcat(buf, tmp, PAGE_SIZE) >= PAGE_SIZE)
> -			goto unlock_buf_done;
> +		len += sysfs_emit_at(buf, len, "WWPN x%llx ",
> +				     nrport->port_name);

[Severity: High]
This is a pre-existing issue, but could this result in a use-after-free of the
nrport pointer?

Looking at lpfc_nvme_info_show() in drivers/scsi/lpfc/lpfc_attr.c, the nrport
pointer is fetched from rport->remoteport under ndlp->lock. However, the lock
is released right before nrport is dereferenced to access fields like
port_state, port_name, node_name, and port_id.

If lpfc_nvme_unregister_port() runs concurrently (such as during a connection
reset or hot-unplug), it explicitly does not hold fc_nodes_list_lock when
retrieving rport->remoteport and calling nvme_fc_unregister_remoteport().

Does anything prevent the remote port from being unregistered and freed by the
NVMe FC transport while this sysfs file is being read, potentially leaving
nrport as a dangling pointer here?

> -		scnprintf(tmp, sizeof(tmp), "WWNN x%llx ",
> -			  nrport->node_name);
> -		if (strlcat(buf, tmp, PAGE_SIZE) >= PAGE_SIZE)
> -			goto unlock_buf_done;
> +		len += sysfs_emit_at(buf, len, "WWNN x%llx ",
> +				     nrport->node_name);

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=5