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

[sashiko-bot@kernel.org]

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