On Tue, 19 May 2026 15:06:15 -0400
Michael Bommarito <[email protected]> wrote:
> An adjacent Fibre Channel fabric actor that can deliver an FPIN ELS
> frame to an lpfc or qla2xxx Linux initiator can trigger a non-return
> in the generic FC transport. This is not a local userspace or IP
> network path; the attacker must be able to inject fabric traffic, for
> example as a compromised switch or fabric controller, or as a same-zone
> N_Port on a fabric that permits source spoofing.
>
> The Link-Integrity and Peer-Congestion FPIN walkers used a u8 loop
> counter against the 32-bit on-wire pname_count field, and did not bound
> pname_count by the descriptor body already validated by the TLV walker.
> A pname_count of 256 therefore wraps the counter and keeps the loop
> condition true indefinitely.
>
> Factor the shared pname_list[] walk into one helper, widen the counter
> to u32, and clamp pname_count against the entries that fit in the
> descriptor body before iterating.
>
> Fixes: 3dcfe0de5a97 ("scsi: fc: Parse FPIN packets and update statistics")
> Cc: [email protected]
> Assisted-by: Claude:claude-opus-4-7
> Signed-off-by: Michael Bommarito <[email protected]>
> ---
> Changes in v3:
> - State the fabric-adjacent threat model explicitly in the commit
> message and clarify that this is not local userspace or IP-network
> reachable.
> - Use min_t(u32, ...) for the pname_count clamp, as Christoph suggested.
> - Use FC_TLV_DESC_LENGTH_FROM_SZ() instead of open-coding the descriptor
> body length calculation.
> - Factor the duplicate LI and peer-congestion pname walker into a common
> helper while preserving the LI-only host-stat update.
>
> Changes in v2:
> - Drop the redundant cover letter shipped with v1. A single-patch send
> does not need one, and the v1 cover carried stale draft markers.
>
> drivers/scsi/scsi_transport_fc.c | 77 +++++++++++++++++---------------
> 1 file changed, 41 insertions(+), 36 deletions(-)
>
> diff --git a/drivers/scsi/scsi_transport_fc.c
> b/drivers/scsi/scsi_transport_fc.c
> index dce95e361daf0..0684d8c69c3c6 100644
> --- a/drivers/scsi/scsi_transport_fc.c
> +++ b/drivers/scsi/scsi_transport_fc.c
> @@ -737,6 +737,37 @@ fc_cn_stats_update(u16 event_type, struct fc_fpin_stats
> *stats)
> }
> }
>
> +static void
> +fc_fpin_pname_stats_update(struct Scsi_Host *shost,
> + struct fc_rport *attach_rport, u16 event_type,
> + u32 desc_len, u32 fixed_len, u32 pname_count,
> + __be64 *pname_list,
> + void (*stats_update)(u16 event_type,
> + struct fc_fpin_stats *stats))
> +{
> + u32 i, max_count;
> + struct fc_rport *rport;
> + u64 wwpn;
> +
> + if (desc_len < fixed_len)
> + max_count = 0;
> + else
> + max_count = (desc_len - fixed_len) / sizeof(pname_list[0]);
> + pname_count = min_t(u32, pname_count, max_count);
No min_t() please, everything is unsigned so min() in fine.
The above might even more readable without the extra variable:
if (desc_len < fixed_len)
pname_count = min(pname_count, (desc_len - fixed_len) /
sizeof(pname_list[0]));
If you think the line is too long s/pname_count/count/g
-- David
> +
> + for (i = 0; i < pname_count; i++) {
> + wwpn = be64_to_cpu(pname_list[i]);
> + rport = fc_find_rport_by_wwpn(shost, wwpn);
> + if (rport &&
> + (rport->roles & FC_PORT_ROLE_FCP_TARGET ||
> + rport->roles & FC_PORT_ROLE_NVME_TARGET)) {
> + if (rport == attach_rport)
> + continue;
> + stats_update(event_type, &rport->fpin_stats);
> + }
> + }
> +}
> +
> /*
> * fc_fpin_li_stats_update - routine to update Link Integrity
> * event statistics.
> @@ -747,13 +778,11 @@ fc_cn_stats_update(u16 event_type, struct fc_fpin_stats
> *stats)
> static void
> fc_fpin_li_stats_update(struct Scsi_Host *shost, struct fc_tlv_desc *tlv)
> {
> - u8 i;
> struct fc_rport *rport = NULL;
> struct fc_rport *attach_rport = NULL;
> struct fc_host_attrs *fc_host = shost_to_fc_host(shost);
> struct fc_fn_li_desc *li_desc = (struct fc_fn_li_desc *)tlv;
> u16 event_type = be16_to_cpu(li_desc->event_type);
> - u64 wwpn;
>
> rport = fc_find_rport_by_wwpn(shost,
> be64_to_cpu(li_desc->attached_wwpn));
> @@ -764,22 +793,11 @@ fc_fpin_li_stats_update(struct Scsi_Host *shost, struct
> fc_tlv_desc *tlv)
> fc_li_stats_update(event_type, &attach_rport->fpin_stats);
> }
>
> - if (be32_to_cpu(li_desc->pname_count) > 0) {
> - for (i = 0;
> - i < be32_to_cpu(li_desc->pname_count);
> - i++) {
> - wwpn = be64_to_cpu(li_desc->pname_list[i]);
> - rport = fc_find_rport_by_wwpn(shost, wwpn);
> - if (rport &&
> - (rport->roles & FC_PORT_ROLE_FCP_TARGET ||
> - rport->roles & FC_PORT_ROLE_NVME_TARGET)) {
> - if (rport == attach_rport)
> - continue;
> - fc_li_stats_update(event_type,
> - &rport->fpin_stats);
> - }
> - }
> - }
> + fc_fpin_pname_stats_update(shost, attach_rport, event_type,
> + be32_to_cpu(li_desc->desc_len),
> + FC_TLV_DESC_LENGTH_FROM_SZ(*li_desc),
> + be32_to_cpu(li_desc->pname_count),
> + li_desc->pname_list, fc_li_stats_update);
>
> if (fc_host->port_name == be64_to_cpu(li_desc->attached_wwpn))
> fc_li_stats_update(event_type, &fc_host->fpin_stats);
> @@ -827,13 +845,11 @@ static void
> fc_fpin_peer_congn_stats_update(struct Scsi_Host *shost,
> struct fc_tlv_desc *tlv)
> {
> - u8 i;
> struct fc_rport *rport = NULL;
> struct fc_rport *attach_rport = NULL;
> struct fc_fn_peer_congn_desc *pc_desc =
> (struct fc_fn_peer_congn_desc *)tlv;
> u16 event_type = be16_to_cpu(pc_desc->event_type);
> - u64 wwpn;
>
> rport = fc_find_rport_by_wwpn(shost,
> be64_to_cpu(pc_desc->attached_wwpn));
> @@ -844,22 +860,11 @@ fc_fpin_peer_congn_stats_update(struct Scsi_Host *shost,
> fc_cn_stats_update(event_type, &attach_rport->fpin_stats);
> }
>
> - if (be32_to_cpu(pc_desc->pname_count) > 0) {
> - for (i = 0;
> - i < be32_to_cpu(pc_desc->pname_count);
> - i++) {
> - wwpn = be64_to_cpu(pc_desc->pname_list[i]);
> - rport = fc_find_rport_by_wwpn(shost, wwpn);
> - if (rport &&
> - (rport->roles & FC_PORT_ROLE_FCP_TARGET ||
> - rport->roles & FC_PORT_ROLE_NVME_TARGET)) {
> - if (rport == attach_rport)
> - continue;
> - fc_cn_stats_update(event_type,
> - &rport->fpin_stats);
> - }
> - }
> - }
> + fc_fpin_pname_stats_update(shost, attach_rport, event_type,
> + be32_to_cpu(pc_desc->desc_len),
> + FC_TLV_DESC_LENGTH_FROM_SZ(*pc_desc),
> + be32_to_cpu(pc_desc->pname_count),
> + pc_desc->pname_list, fc_cn_stats_update);
> }
>
> /*
