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

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

[Yehyeong Lee]

bfa_fcs_lport_scn_process_rscn() takes the length of the frame it was
given 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 of BFA_UF_BUFSZ, which holds 569
events after the fchs_s and the RSCN header. The loop reads up to 63 kB
past that buffer, and the duplicate scan nested inside it is quadratic in
num_entries, so a single frame can also spin the receive path through
more than a hundred million iterations.

Bound the walk by both lengths that matter.

The first is the received frame length, which is already here:
bfa_fcs_uf_recv() takes it from the completion with bfa_uf_get_frmlen()
and it is passed down through bfa_fcs_fabric_uf_recv() and
bfa_fcs_lport_uf_recv() to this function. Work out how many events the
received frame can hold and take the smaller of that and what the payload
claims.

The second is what the buffer physically holds. That frame length is not
itself bounded by the buffer: uf_recv() assigns it straight from the
completion message, "uf->data_len = m->xfr_len", and only warns when it
is below sizeof(struct fchs_s), so a 16-bit transfer length larger than
BFA_UF_BUFSZ is accepted. Cap the event count at BFA_UF_BUFSZ as well, so
the bound does not rest on an invariant the completion path does not
enforce.

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.

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 a BFA_UF_BUFSZ 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>
---
 drivers/scsi/bfa/bfa_fcs_lport.c | 20 +++++++++++++++++++-
 1 file changed, 19 insertions(+), 1 deletion(-)

diff --git a/drivers/scsi/bfa/bfa_fcs_lport.c b/drivers/scsi/bfa/bfa_fcs_lport.c
index 8c9d423129c07..bb1a821716f38 100644
--- a/drivers/scsi/bfa/bfa_fcs_lport.c
+++ b/drivers/scsi/bfa/bfa_fcs_lport.c
@@ -5533,10 +5533,13 @@ 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, 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);
+	int             buf_cap = (BFA_UF_BUFSZ - pl_off) /
+				  sizeof(rscn->event[0]);
 
 	num_entries =
 		(be16_to_cpu(rscn->payldlen) -
@@ -5544,6 +5547,21 @@ bfa_fcs_lport_scn_process_rscn(struct bfa_fcs_lport_s *port,
 
 	bfa_trc(port->fcs, num_entries);
 
+	/*
+	 * the event list cannot run past the frame that was received, nor
+	 * past what the unsolicited frame buffer physically holds
+	 */
+	if (len > pl_off)
+		max_entries = (len - pl_off) / sizeof(rscn->event[0]);
+
+	if (max_entries > buf_cap)
+		max_entries = buf_cap;
+
+	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] 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: 32-bit integer underflow parses garbage RSCN events
- [High] scsi: bfa: OOB read in RSCN event parsing due to unhandled VFT header shift
--

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

This commit attempts to prevent an unbounded loop when processing RSCN
events by limiting the event list size to the received frame length and
the physical buffer capacity.

> diff --git a/drivers/scsi/bfa/bfa_fcs_lport.c b/drivers/scsi/bfa/bfa_fcs_lport.c
> index 8c9d423129c07..bb1a821716f38 100644
> --- a/drivers/scsi/bfa/bfa_fcs_lport.c
> +++ b/drivers/scsi/bfa/bfa_fcs_lport.c
[ ... ]
> @@ -5533,10 +5533,13 @@ 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, 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);
> +	int             buf_cap = (BFA_UF_BUFSZ - pl_off) /
> +				  sizeof(rscn->event[0]);

[Severity: High]
Does this buf_cap calculation overestimate the remaining buffer size?

In bfa_fcs_uf_recv(), if a VFT header is present, the fchs pointer is advanced
by 8 bytes:

drivers/scsi/bfa/bfa_fcs.c:bfa_fcs_uf_recv() {
    ...
		/*
		 * skip vft header
		 */
		fchs = (struct fchs_s *) (vft + 1);
		len -= sizeof(struct fc_vft_s);
    ...
}

Because the pointer might be shifted 8 bytes into the DMA buffer, the remaining
capacity from fchs to the end of the buffer is BFA_UF_BUFSZ - 8, not
BFA_UF_BUFSZ.

If a remote peer sends a crafted RSCN frame with a VFT header and a spoofed
payload length, buf_cap will be overestimated by 8 bytes. Parsing these extra
events could result in an 8-byte out-of-bounds read past the end of the buffer.

>  
>  	num_entries =
>  		(be16_to_cpu(rscn->payldlen) -

[Severity: High]
If a malformed network frame has payldlen < 4, does the subtraction underflow?

The calculation (be16_to_cpu(rscn->payldlen) - sizeof(u32)) promotes the
16-bit length to an unsigned size_t. On 32-bit architectures, this underflows
to a huge positive number. Divided by 4 (sizeof(rscn->event[0])), it yields
1073741823, which is then assigned to the 32-bit signed int num_entries.

> @@ -5544,6 +5547,21 @@ bfa_fcs_lport_scn_process_rscn(struct bfa_fcs_lport_s *port,
>  
>  	bfa_trc(port->fcs, num_entries);
>  
> +	/*
> +	 * the event list cannot run past the frame that was received, nor
> +	 * past what the unsolicited frame buffer physically holds
> +	 */
> +	if (len > pl_off)
> +		max_entries = (len - pl_off) / sizeof(rscn->event[0]);
> +
> +	if (max_entries > buf_cap)
> +		max_entries = buf_cap;
> +
> +	if (num_entries > max_entries)
> +		num_entries = max_entries;

[Severity: High]
By clamping the huge underflowed num_entries to max_entries without validating
that payldlen >= 4, does the loop now parse padding bytes or uninitialized
buffer data as legitimate network events?

If the received frame contains padding bytes (len > pl_off), max_entries will
be greater than 0, causing the code to process garbage as valid RSCN events
instead of rejecting the invalid frame entirely.

> +
> +	bfa_trc(port->fcs, max_entries);
> +
>  	port->stats.num_rscn++;

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