[PATCH v2] scsi: bfa: bound the RSCN event list by the received frame length

[PATCH v2] scsi: bfa: bound the RSCN event list by the received frame length

[Yehyeong Lee]

bfa_fcs_lport_scn_process_rscn() is given the length of the frame it has
to parse and never looks at it. The number of events it walks comes only
from the payload length the peer put in the frame:

	num_entries =
		(be16_to_cpu(rscn->payldlen) -
		 sizeof(u32)) / sizeof(rscn->event[0]);

	for (i = 0; i < num_entries; i++) {
		rscn_pid = rscn->event[i].portid;

payldlen is a 16-bit field, so num_entries can be 16382 while the frame
sits in an unsolicited frame buffer that holds 569 events after the
fchs_s and the RSCN header. The loop reads up to 63 kB past the frame it
was handed, and the duplicate scan nested inside it is quadratic in
num_entries, so one frame can also take the receive path through more
than a hundred million iterations.

The length that bounds this is already here. bfa_fcs_uf_recv() takes it
from the completion with bfa_uf_get_frmlen() and it reaches this function
through bfa_fcs_fabric_uf_recv() and bfa_fcs_lport_uf_recv(). Work out
how many events the received frame can hold and take the smaller of that
and what the payload claims. A frame whose payload fits is parsed exactly
as before; one that claims more is parsed only as far as it goes, which
also caps the duplicate scan.

Reject a payload length below the four-byte RSCN header it is measured
from as well, instead of subtracting past it.

The same shape is in lpfc_els.c, where lpfc_els_rcv_rscn() and
lpfc_rscn_payload_check() walk the payload length without comparing it to
the frame either.

I have no Brocade hardware; this is from the code, with the arithmetic and
the loop bounds modelled against an unsolicited frame buffer under KASAN.

Fixes: 7725ccfda597 ("[SCSI] bfa: Brocade BFA FC SCSI driver")
Cc: stable@vger.kernel.org
Signed-off-by: Yehyeong Lee <yhlee@isslab.korea.ac.kr>
---
v2: bound by the received frame length only (drop the buffer-capacity
term, which did not account for the VFT header offset); reject payloads
shorter than the RSCN header.  Addresses review feedback.

 drivers/scsi/bfa/bfa_fcs_lport.c | 19 +++++++++++++++----
 1 file changed, 15 insertions(+), 4 deletions(-)

diff --git a/drivers/scsi/bfa/bfa_fcs_lport.c b/drivers/scsi/bfa/bfa_fcs_lport.c
index 8c9d423129c07..49bcb76c118b6 100644
--- a/drivers/scsi/bfa/bfa_fcs_lport.c
+++ b/drivers/scsi/bfa/bfa_fcs_lport.c
@@ -5533,17 +5533,28 @@ bfa_fcs_lport_scn_process_rscn(struct bfa_fcs_lport_s *port,
 			struct fchs_s *fchs, u32 len)
 {
 	struct fc_rscn_pl_s *rscn = (struct fc_rscn_pl_s *) (fchs + 1);
-	int             num_entries;
+	int             num_entries = 0, max_entries = 0;
 	u32        rscn_pid;
 	bfa_boolean_t   nsquery = BFA_FALSE, found;
 	int             i = 0, j;
+	u32             pl_off = sizeof(struct fchs_s) + sizeof(*rscn);
+	u16             payldlen = be16_to_cpu(rscn->payldlen);
 
-	num_entries =
-		(be16_to_cpu(rscn->payldlen) -
-		 sizeof(u32)) / sizeof(rscn->event[0]);
+	/* payldlen spans a four-byte header and the event list */
+	if (payldlen >= sizeof(u32))
+		num_entries = (payldlen - sizeof(u32)) / sizeof(rscn->event[0]);
 
 	bfa_trc(port->fcs, num_entries);
 
+	/* the event list cannot run past the frame that was received */
+	if (len > pl_off)
+		max_entries = (len - pl_off) / sizeof(rscn->event[0]);
+
+	if (num_entries > max_entries)
+		num_entries = max_entries;
+
+	bfa_trc(port->fcs, max_entries);
+
 	port->stats.num_rscn++;
 
 	bfa_fcs_lport_scn_send_ls_acc(port, fchs);

base-commit: 2f2f7eeef8b0a1a9b9ca998231419873ccae8d4e
-- 
2.43.0

Re: [PATCH v2] scsi: bfa: bound the RSCN event list by the received frame length

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] scsi: bfa: integer underflow in bfa_fcs_uf_recv bypasses RSCN bounds check
- [Medium] scsi: bfa: missing endianness conditionals for fc_rscn_event_s bitfields
--

commit af527b1c9309cc98e3d17a397e7a493ebc42ff03
Author: Yehyeong Lee <yhlee@isslab.korea.ac.kr>
Subject: scsi: bfa: bound the RSCN event list by the received frame length

This commit bounds the RSCN event list processing by comparing the payload
length to the actual received frame length. It is intended to prevent
out-of-bounds reads when processing maliciously crafted or malformed RSCN
frames with oversized payload length claims.

> diff --git a/drivers/scsi/bfa/bfa_fcs_lport.c b/drivers/scsi/bfa/bfa_fcs_lport.c
> index 8c9d423129c07..49bcb76c118b6 100644
> --- a/drivers/scsi/bfa/bfa_fcs_lport.c
> +++ b/drivers/scsi/bfa/bfa_fcs_lport.c
> @@ -5533,17 +5533,28 @@ bfa_fcs_lport_scn_process_rscn(struct bfa_fcs_lport_s *port,
>  			struct fchs_s *fchs, u32 len)
>  {
>  	struct fc_rscn_pl_s *rscn = (struct fc_rscn_pl_s *) (fchs + 1);
> -	int             num_entries;
> +	int             num_entries = 0, max_entries = 0;
>  	u32        rscn_pid;
>  	bfa_boolean_t   nsquery = BFA_FALSE, found;
>  	int             i = 0, j;
> +	u32             pl_off = sizeof(struct fchs_s) + sizeof(*rscn);
> +	u16             payldlen = be16_to_cpu(rscn->payldlen);
>  
> -	num_entries =
> -		(be16_to_cpu(rscn->payldlen) -
> -		 sizeof(u32)) / sizeof(rscn->event[0]);
> +	/* payldlen spans a four-byte header and the event list */
> +	if (payldlen >= sizeof(u32))
> +		num_entries = (payldlen - sizeof(u32)) / sizeof(rscn->event[0]);
>  
>  	bfa_trc(port->fcs, num_entries);
>  
> +	/* the event list cannot run past the frame that was received */
> +	if (len > pl_off)
> +		max_entries = (len - pl_off) / sizeof(rscn->event[0]);

[Severity: High]
Does this code fully prevent out-of-bounds reads if the received frame length
is underflowed?

When a truncated frame with a VFT header is passed to bfa_fcs_uf_recv():

drivers/scsi/bfa/bfa_fcs.c:bfa_fcs_uf_recv() {
	u16	len = bfa_uf_get_frmlen(uf);
	...
	if (fchs->routing == FC_RTG_EXT_HDR &&
	    fchs->cat_info == FC_CAT_VFT_HDR) {
		...
		fchs = (struct fchs_s *) (vft + 1);
		len -= sizeof(struct fc_vft_s);
	...
	bfa_fcs_fabric_uf_recv(fabric, fchs, len);
}

If the frame length is less than 8 bytes, len underflows to a very large
16-bit value (e.g., 65530). This massive length is passed down the call chain
into bfa_fcs_lport_scn_process_rscn() as the u32 len parameter.

Since the underflowed len easily passes the (len > pl_off) bounds check
introduced here, max_entries becomes erroneously large.

Doesn't this bypass the security fix and allow an attacker-controlled payload
length to trigger an out-of-bounds read up to 63 kB past the frame buffer?

> +
> +	if (num_entries > max_entries)
> +		num_entries = max_entries;

[Severity: Medium]
Are the event bitfields safe to parse directly on big-endian architectures?

Unlike the other Fibre Channel headers in this driver, the structure
fc_rscn_event_s mapping directly to the network payload lacks big-endian
conditional guards:

drivers/scsi/bfa/bfa_fc.h:
struct fc_rscn_event_s {
	u32	format:2;
	u32	qualifier:4;
	u32	resvd:2;
	u32	portid:24;
};

When bfa_fcs_lport_scn_process_rscn() iterates over num_entries, it extracts
rscn->event[i].portid and rscn->event[i].format.

On big-endian systems, won't the bitfields be packed in reverse order compared
to little-endian, causing the driver to extract the wrong RSCN address format
and incorrectly pad the 24-bit portid?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261016065827.1011210-1-yhlee@isslab.korea.ac.kr?part=1