Re: [PATCH v3] scsi: scsi_transport_fc: widen FPIN pname walker counter to u32

[David Laight <david.laight.linux@gmail.com>]

On Tue, 19 May 2026 15:06:15 -0400
Michael Bommarito <michael.bommarito@gmail.com> 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: stable@vger.kernel.org
> Assisted-by: Claude:claude-opus-4-7
> Signed-off-by: Michael Bommarito <michael.bommarito@gmail.com>
> ---
> 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);
>  }
>  
>  /*