[PATCH v4 0/3] scsi: bfa: bound the unsolicited frame receive path

[PATCH v4 0/3] scsi: bfa: bound the unsolicited frame receive path

[Yehyeong Lee]

Three fixes in the bfa unsolicited frame receive path. Together they close
the length the path trusts, from where it enters to where it is used.

 1  uf_recv() takes the received length straight from the firmware's
    completion message and only warns if it is below sizeof(struct
    fchs_s). It is a 16-bit field and nothing caps it at the buffer that
    was posted, so every consumer below can be handed a length larger
    than the frame. The worst of them is bfa_fcs_lport_echo(), which uses
    it as a memcpy size into the fcxp request payload buffer, so that one
    is an out-of-bounds write and not just a read. Cap the length at the
    buffer that was posted.
 2  bfa_fcs_uf_recv() steps over a virtual fabric tagging header without
    checking that the frame is long enough to hold it. The length is a
    u16, so a frame too short for its own VFT header wraps it. Drop such
    a frame, the way the block above it drops a frame with an unknown
    virtual fabric id.
 3  bfa_fcs_lport_scn_process_rscn() is handed that length and ignores it,
    taking the number of RSCN events to walk from the peer's payload
    length field alone. Bound the walk by the received frame length, and
    reject a payload length below the RSCN header it is measured from.

Patch 1 is the source and patches 2 and 3 are two of the sinks; each
stands on its own, and 1 is first because it is the one that makes the
length the others use meaningful.

None of this is tested on hardware; I have no Brocade adapter. All three
were driven through a KUnit harness on a KASAN kernel that injects a frame
completion into uf_recv(), a frame into bfa_fcs_uf_recv(), and frames into
the RSCN and ECHO handlers directly. Without patch 1 a reported length of
2400 reaches bfa_fcs_lport_echo() and its memcpy runs 72 bytes past the
2304-byte request buffer; with it the length is capped at the posted
buffer and a normal frame of FC_MAX_PDUSZ passes through untouched.
Without patch 2 a six-byte VFT-tagged frame hands the FCS a length of
65534. Without patch 3 an RSCN whose payload length exceeds the frame
reads past the frame buffer. With all three, a legitimate RSCN, including
one that fills the frame exactly, parses unchanged.

v4: adds the missing cap on the frame length itself in uf_recv(); v3
    bounded the RSCN walk and the VFT underflow but still trusted a
    firmware-reported length that bfa_fcs_lport_echo() uses to drive an
    out-of-bounds memcpy.
v3: added the VFT-header length underflow guard.
v2: bound the RSCN walk by the received frame length only (dropped a
    buffer-capacity term that did not account for the VFT header offset);
    reject payloads shorter than the RSCN header.

Yehyeong Lee (3):
  scsi: bfa: clamp the received unsolicited frame length to the buffer
  scsi: bfa: drop unsolicited frames shorter than their VFT header
  scsi: bfa: bound the RSCN event list by the received frame length

 drivers/scsi/bfa/bfa_fcs.c       |  5 +++++
 drivers/scsi/bfa/bfa_fcs_lport.c | 19 +++++++++++++++----
 drivers/scsi/bfa/bfa_svc.c       |  2 +-
 3 files changed, 21 insertions(+), 5 deletions(-)


base-commit: 2f2f7eeef8b0a1a9b9ca998231419873ccae8d4e
-- 
2.43.0

[PATCH v4 1/3] scsi: bfa: clamp the received unsolicited frame length to the buffer

[Yehyeong Lee]

uf_recv() takes the received length of an unsolicited frame straight from
the firmware's completion message:

	uf->data_len = m->xfr_len;

	WARN_ON(uf->data_len < sizeof(struct fchs_s));

The only check is that lower bound, and it only warns. xfr_len is 16 bits,
so a length of up to 65535 is accepted for a frame that was DMAed into a
buffer of sizeof(struct bfa_uf_buf_s), which is what claim_uf_post_msgs()
posted to the firmware in the first place:

	buf_len = sizeof(struct bfa_uf_buf_s);
	uf_bp_msg->buf_len = cpu_to_be16(buf_len);
	bfa_alen_set(&uf_bp_msg->alen, buf_len, ufm_pbs_pa(ufm, i));

bfa_uf_get_frmlen() hands that length to the FCS, which passes it down to
every unsolicited frame consumer, and they believe it. The worst of them
is bfa_fcs_lport_echo(), which takes the length as a memcpy size:

	pyld_len = rx_len - sizeof(struct fchs_s);
	if (pyld_len > len)
		memcpy(((u8 *) bfa_fcxp_get_reqbuf(fcxp)) +
			sizeof(struct fc_echo_s), (echo + 1),
			(pyld_len - sizeof(struct fc_echo_s)));

The destination is the fcxp request payload buffer, which is
BFA_FCXP_MAX_IBUF_SZ, and the response payload buffer for the same fcxp
sits immediately after it. A reported length above 2328 makes that copy
run past the request buffer, so this is a write, not a read.

Cap the length at the buffer that was posted. A normal frame is not
affected: FC_MAX_PDUSZ is 2112 and the posted buffer is larger than that,
so the cap only takes effect for a length the buffer could never have
held.

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_svc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/scsi/bfa/bfa_svc.c b/drivers/scsi/bfa/bfa_svc.c
index df33afaaa6735..1ccf2e994a671 100644
--- a/drivers/scsi/bfa/bfa_svc.c
+++ b/drivers/scsi/bfa/bfa_svc.c
@@ -5453,7 +5453,7 @@ uf_recv(struct bfa_s *bfa, struct bfi_uf_frm_rcvd_s *m)
 	list_del(&uf->qe);	/* dequeue from posted queue */
 
 	uf->data_ptr = buf;
-	uf->data_len = m->xfr_len;
+	uf->data_len = min_t(u16, m->xfr_len, sizeof(struct bfa_uf_buf_s));
 
 	WARN_ON(uf->data_len < sizeof(struct fchs_s));
 
-- 
2.43.0

Re: [PATCH v4 1/3] scsi: bfa: clamp the received unsolicited frame length to the buffer

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] scsi: bfa: integer underflow causes massive OOB DMA read in bfa_fcs_lport_echo
- [High] scsi: bfa: missing bounds check on firmware-supplied buf_tag in uf_recv()
--

commit 4545309c07b7ce26f9ea539ffa38c0ea8612717f
Author: Yehyeong Lee <yhlee@isslab.korea.ac.kr>

scsi: bfa: clamp the received unsolicited frame length to the buffer

The commit addresses an issue where uf_recv() blindly accepts the
unsolicited frame length from the firmware. By capping uf->data_len to
the size of the posted buffer, it prevents out-of-bounds writes in
downstream consumers.

> diff --git a/drivers/scsi/bfa/bfa_svc.c b/drivers/scsi/bfa/bfa_svc.c
> index df33afaaa6735..1ccf2e994a671 100644
> --- a/drivers/scsi/bfa/bfa_svc.c
> +++ b/drivers/scsi/bfa/bfa_svc.c
> @@ -5453,7 +5453,7 @@ uf_recv(struct bfa_s *bfa, struct bfi_uf_frm_rcvd_s *m)

[Severity: High]
Since this patch addresses untrusted firmware input by clamping m->xfr_len,
is it safe to use m->buf_tag as a direct array index?

Earlier in this same function:

drivers/scsi/bfa/bfa_svc.c:uf_recv() {
    ...
	u16 uf_tag = m->buf_tag;
	struct bfa_uf_s *uf = &ufm->uf_list[uf_tag];
    ...
	uf_buf = (struct bfa_uf_buf_s *)
			bfa_mem_get_dmabuf_kva(ufm, uf_tag, uf->pb_len);
    ...
}

If a buggy or malicious firmware provides an out-of-bounds buf_tag, won't this
lead to out-of-bounds array accesses to ufm->uf_list and out-of-bounds DMA
segment calculations?

>  	list_del(&uf->qe);	/* dequeue from posted queue */
>  
>  	uf->data_ptr = buf;
> -	uf->data_len = m->xfr_len;
> +	uf->data_len = min_t(u16, m->xfr_len, sizeof(struct bfa_uf_buf_s));
>  
>  	WARN_ON(uf->data_len < sizeof(struct fchs_s));
>  

[Severity: High]
If the received frame is truncated and m->xfr_len is smaller than
sizeof(struct fchs_s) (24 bytes), this warns but still processes the frame.

Will this lead to an integer underflow later? In bfa_fcs_lport_echo(),
pyld_len is calculated as:

drivers/scsi/bfa/bfa_fcs_lport.c:bfa_fcs_lport_echo() {
    ...
    pyld_len = rx_len - sizeof(struct fchs_s);
    ...
}

If rx_len is less than 24, pyld_len underflows. Since pyld_len is an int,
this evaluates to a negative number, bypassing the subsequent check:

    if (pyld_len > len)

The negative pyld_len is then passed to bfa_fcxp_send(), which takes a u32
reqlen. Does this implicit cast to a massive length (~4.29GB) result in a huge
out-of-bounds DMA read when submitted to the firmware?

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

[PATCH v4 2/3] scsi: bfa: drop unsolicited frames shorter than their VFT header

[Yehyeong Lee]

bfa_fcs_uf_recv() keeps the received frame length in a u16 and, for a
frame that carries a virtual fabric tagging header, steps over that
header:

	fchs = (struct fchs_s *) (vft + 1);
	len -= sizeof(struct fc_vft_s);

Nothing has checked that the frame is long enough to hold the header it
says it has. A frame too short to hold its own VFT header makes the
subtraction wrap, so len comes out near 65530 instead of going negative,
and that length is what bfa_fcs_fabric_uf_recv() and
bfa_fcs_lport_uf_recv() then hand to every ELS handler. Any of them that
believes it reads up to 64 kB past the frame buffer.

Drop the frame instead, the same way the block right above drops a frame
whose virtual fabric id is unknown.

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.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/scsi/bfa/bfa_fcs.c b/drivers/scsi/bfa/bfa_fcs.c
index 9b57312f43f50..711d2ed83a7aa 100644
--- a/drivers/scsi/bfa/bfa_fcs.c
+++ b/drivers/scsi/bfa/bfa_fcs.c
@@ -1552,6 +1552,11 @@ bfa_fcs_uf_recv(void *cbarg, struct bfa_uf_s *uf)
 		/*
 		 * skip vft header
 		 */
+		if (len < sizeof(struct fc_vft_s)) {
+			bfa_uf_free(uf);
+			return;
+		}
+
 		fchs = (struct fchs_s *) (vft + 1);
 		len -= sizeof(struct fc_vft_s);
 
-- 
2.43.0

Re: [PATCH v4 2/3] scsi: bfa: drop unsolicited frames shorter than their VFT header

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] scsi: bfa: Incomplete bounds checking in bfa_fcs_uf_recv leads to out-of-bounds reads
--

commit 5c4d11be4a4f404e42897f255d18569a1acd5b93
Author: Yehyeong Lee <yhlee@isslab.korea.ac.kr>

scsi: bfa: drop unsolicited frames shorter than their VFT header

This patch fixes an integer underflow by checking that the received frame
length is at least the size of the virtual fabric tagging (VFT) header
before attempting to step over it in bfa_fcs_uf_recv().

> diff --git a/drivers/scsi/bfa/bfa_fcs.c b/drivers/scsi/bfa/bfa_fcs.c
> index 9b57312f43f50..711d2ed83a7aa 100644
> --- a/drivers/scsi/bfa/bfa_fcs.c
> +++ b/drivers/scsi/bfa/bfa_fcs.c
> @@ -1552,6 +1552,11 @@ bfa_fcs_uf_recv(void *cbarg, struct bfa_uf_s *uf)
>  		/*
>  		 * skip vft header
>  		 */
> +		if (len < sizeof(struct fc_vft_s)) {
> +			bfa_uf_free(uf);
> +			return;
> +		}
> +

[Severity: High]
Does this check occur too late to prevent out-of-bounds reads?

Before reaching this new bounds check, the function already accesses fields
from the frame buffer to check for the VFT header:

	if (fchs->routing == FC_RTG_EXT_HDR &&
	    fchs->cat_info == FC_CAT_VFT_HDR) {
		bfa_stats(fcs, uf.tagged);
		vft = bfa_uf_get_frmbuf(uf);
		if (fcs->port_vfid == vft->vf_id)

If the frame is shorter than 8 bytes, could these accesses read uninitialized
or stale DMA memory?

>  		fchs = (struct fchs_s *) (vft + 1);
>  		len -= sizeof(struct fc_vft_s);
>  

Does this code need to verify that the remaining frame length can hold the
mandatory 24-byte inner fchs_s header?

Without checking that the remaining len is at least sizeof(struct fchs_s),
truncated frames are passed to downstream handlers. For example, in
bfa_fcs_fabric_uf_recv(), the pointer is unconditionally cast and accessed:

	u32 pid = fchs->d_id;
	struct fc_els_cmd_s *els_cmd = (struct fc_els_cmd_s *) (fchs + 1);

Could this process stale ghost data as valid protocol commands and confuse the
state machine?

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

[PATCH v4 3/3] 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>
---
 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);
-- 
2.43.0