* Re: [PATCH v3] scsi: scsi_transport_fc: widen FPIN pname walker counter to u32
2026-05-19 19:06 ` [PATCH v3] scsi: scsi_transport_fc: widen FPIN pname walker counter to u32 Michael Bommarito
@ 2026-05-20 12:58 ` David Laight
2026-05-20 13:30 ` [PATCH v4] " Michael Bommarito
1 sibling, 0 replies; 3+ messages in thread
From: David Laight @ 2026-05-20 12:58 UTC (permalink / raw)
To: Michael Bommarito
Cc: Martin K. Petersen, James E.J. Bottomley, Nilesh Javali,
Himanshu Madhani, Shyam Sundar, James Smart, Hannes Reinecke,
John Meneghini, Bryan Gurney, Justin Tee, Christoph Hellwig,
Keith Busch, Kees Cook, linux-scsi, linux-nvme, linux-hardening,
linux-kernel, stable
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);
> }
>
> /*
^ permalink raw reply [flat|nested] 3+ messages in thread* [PATCH v4] scsi: scsi_transport_fc: widen FPIN pname walker counter to u32
2026-05-19 19:06 ` [PATCH v3] scsi: scsi_transport_fc: widen FPIN pname walker counter to u32 Michael Bommarito
2026-05-20 12:58 ` David Laight
@ 2026-05-20 13:30 ` Michael Bommarito
1 sibling, 0 replies; 3+ messages in thread
From: Michael Bommarito @ 2026-05-20 13:30 UTC (permalink / raw)
To: Martin K. Petersen, James E.J. Bottomley
Cc: Nilesh Javali, Himanshu Madhani, Shyam Sundar, James Smart,
Hannes Reinecke, John Meneghini, Bryan Gurney, Justin Tee,
Christoph Hellwig, David Laight, Keith Busch, Kees Cook,
linux-scsi, linux-nvme, linux-hardening, linux-kernel, stable
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 v4:
- Use min() rather than min_t(u32, ...) for the pname_count clamp and
fold away the temporary max_count variable, as David Laight suggested.
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..173ed6373f04b 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;
+ struct fc_rport *rport;
+ u64 wwpn;
+
+ if (desc_len < fixed_len)
+ pname_count = 0;
+ else
+ pname_count = min(pname_count, (desc_len - fixed_len) /
+ sizeof(pname_list[0]));
+
+ 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);
}
/*
--
2.53.0
^ permalink raw reply related [flat|nested] 3+ messages in thread