* [PATCH v3] scsi: scsi_transport_fc: widen FPIN pname walker counter to u32 [not found] <20260518143706.2808177-1-michael.bommarito@gmail.com> @ 2026-05-19 19:06 ` Michael Bommarito 2026-05-20 12:58 ` David Laight 2026-05-20 13:30 ` [PATCH v4] " Michael Bommarito 0 siblings, 2 replies; 6+ messages in thread From: Michael Bommarito @ 2026-05-19 19:06 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, 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 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); + + 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] 6+ messages in thread
* 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; 6+ 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] 6+ 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 2026-05-22 9:22 ` Christoph Hellwig ` (2 more replies) 1 sibling, 3 replies; 6+ 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] 6+ messages in thread
* Re: [PATCH v4] scsi: scsi_transport_fc: widen FPIN pname walker counter to u32 2026-05-20 13:30 ` [PATCH v4] " Michael Bommarito @ 2026-05-22 9:22 ` Christoph Hellwig 2026-05-22 10:11 ` John Garry 2026-05-27 7:14 ` Hannes Reinecke 2 siblings, 0 replies; 6+ messages in thread From: Christoph Hellwig @ 2026-05-22 9:22 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, David Laight, Keith Busch, Kees Cook, linux-scsi, linux-nvme, linux-hardening, linux-kernel, stable Looks good: Reviewed-by: Christoph Hellwig <hch@lst.de> ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v4] scsi: scsi_transport_fc: widen FPIN pname walker counter to u32 2026-05-20 13:30 ` [PATCH v4] " Michael Bommarito 2026-05-22 9:22 ` Christoph Hellwig @ 2026-05-22 10:11 ` John Garry 2026-05-27 7:14 ` Hannes Reinecke 2 siblings, 0 replies; 6+ messages in thread From: John Garry @ 2026-05-22 10:11 UTC (permalink / raw) To: Michael Bommarito, 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 On 20/05/2026 14:30, Michael Bommarito 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> Regardless of nitpick below: Reviewed-by: John Garry <john.g.garry@oracle.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; you could return directly here to avoid extra indentation in else leg > + 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); > + } > + } > +} ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v4] scsi: scsi_transport_fc: widen FPIN pname walker counter to u32 2026-05-20 13:30 ` [PATCH v4] " Michael Bommarito 2026-05-22 9:22 ` Christoph Hellwig 2026-05-22 10:11 ` John Garry @ 2026-05-27 7:14 ` Hannes Reinecke 2 siblings, 0 replies; 6+ messages in thread From: Hannes Reinecke @ 2026-05-27 7:14 UTC (permalink / raw) To: Michael Bommarito, 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 On 5/20/26 15:30, Michael Bommarito 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 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(-) > Reviewed-by: Hannes Reinecke <hare@suse.de> Cheers, Hannes -- Dr. Hannes Reinecke Kernel Storage Architect hare@suse.de +49 911 74053 688 SUSE Software Solutions GmbH, Frankenstr. 146, 90461 Nürnberg HRB 36809 (AG Nürnberg), GF: I. Totev, A. McDonald, W. Knoblich ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2026-05-27 7:14 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <20260518143706.2808177-1-michael.bommarito@gmail.com>
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
2026-05-22 9:22 ` Christoph Hellwig
2026-05-22 10:11 ` John Garry
2026-05-27 7:14 ` Hannes Reinecke
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox