Re: [PATCH v2] 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: 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