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

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

[Yehyeong Lee]

Three fixes in the bfa unsolicited frame receive path. Together they bound
the length the path trusts, at both ends, 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() does not check the frame against the headers it has
    to hold. Below the FC header, bfa_fcs_lport_echo() computes a negative
    payload length and hands it to bfa_fcxp_send(), whose reqlen is a u32,
    so a six-byte frame asks the firmware to send about four gigabytes
    from the request payload address. Below the virtual fabric tagging
    header it advertises, stepping over that header wraps the length.
    Drop both, the way the function already drops a frame whose virtual
    fabric id is unknown.
 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.

Patches 1 and 2 are the source and patch 3 is one of the sinks; each
stands on its own, and the first two come first because they are what
makes the length the third uses meaningful. With both, every consumer
sees a length between the FC header and the posted buffer.

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. Without patch 2 a six-byte frame reaches the
same handler and leaves 4294967278 in the request length it hands the
firmware, and a six-byte 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 header-only frame of 24 bytes, a frame
of FC_MAX_PDUSZ and a legitimate RSCN, including one that fills the frame
exactly, are all unaffected.

v5: also drop frames shorter than the mandatory FC header in
    bfa_fcs_uf_recv(), closing a pre-existing lower-bound underflow that
    reaches bfa_fcs_lport_echo().
v4: added the 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 the headers they must
    hold
  scsi: bfa: bound the RSCN event list by the received frame length

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


base-commit: 2f2f7eeef8b0a1a9b9ca998231419873ccae8d4e
-- 
2.43.0

[PATCH v5 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

[PATCH v5 2/3] scsi: bfa: drop unsolicited frames shorter than the headers they must hold

[Yehyeong Lee]

bfa_fcs_uf_recv() keeps the received frame length in a u16 and hands it,
with a pointer to the frame, to every unsolicited frame consumer. Two
lengths are never checked.

The frame is not checked against the FC header that every consumer reads.
bfa_fcs_fabric_uf_recv() takes fchs->d_id and els_cmd->els_code out of it
before dispatching, and the handlers below take the length as a size. The
worst of those is bfa_fcs_lport_echo():

	pyld_len = rx_len - sizeof(struct fchs_s);

pyld_len is an int, so a frame shorter than the FC header makes it
negative. The memcpy below is skipped because the comparison it guards is
signed, but the value is then passed to bfa_fcxp_send(), whose reqlen
parameter is a u32:

	reqi->req_tot_len = reqlen;
	...
	send_req->req_len = cpu_to_be32(reqi->req_tot_len);
	bfa_alen_set(&send_req->req_alen, reqi->req_tot_len,
		     BFA_FCXP_REQ_PLD_PA(fcxp));

so a six-byte frame asks the firmware to send roughly four gigabytes
starting at the fcxp request payload address. The response side of the
same function sanity checks its length against BFA_FCXP_MAX_LBUF_SZ; the
request side does not.

The frame is also not checked against the virtual fabric tagging header
it may advertise. For a tagged frame the function steps over that header:

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

and a frame too short to hold the header makes that subtraction wrap, so
the length comes out near 65530 instead of going negative.

Drop a frame that cannot hold the headers it has to hold: the FC header
before anything reads it, and, for a tagged frame, the VFT header plus
the FC header that has to follow it. Both drops use the idiom already in
this function for a frame whose virtual fabric id is unknown. A normal
frame is not affected; the FC header is always 24 bytes and a tagged one
carries it after the 8-byte VFT header.

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 | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

diff --git a/drivers/scsi/bfa/bfa_fcs.c b/drivers/scsi/bfa/bfa_fcs.c
index 9b57312f43f50..8bb92f935a40f 100644
--- a/drivers/scsi/bfa/bfa_fcs.c
+++ b/drivers/scsi/bfa/bfa_fcs.c
@@ -1527,6 +1527,14 @@ bfa_fcs_uf_recv(void *cbarg, struct bfa_uf_s *uf)
 	struct fc_vft_s *vft;
 	struct bfa_fcs_fabric_s *fabric;
 
+	/*
+	 * every consumer reads the FC header, so the frame has to hold one
+	 */
+	if (len < sizeof(struct fchs_s)) {
+		bfa_uf_free(uf);
+		return;
+	}
+
 	/*
 	 * check for VFT header
 	 */
@@ -1550,8 +1558,13 @@ bfa_fcs_uf_recv(void *cbarg, struct bfa_uf_s *uf)
 		}
 
 		/*
-		 * skip vft header
+		 * skip vft header, which the inner FC header has to follow
 		 */
+		if (len < sizeof(struct fc_vft_s) + sizeof(struct fchs_s)) {
+			bfa_uf_free(uf);
+			return;
+		}
+
 		fchs = (struct fchs_s *) (vft + 1);
 		len -= sizeof(struct fc_vft_s);
 
-- 
2.43.0

[PATCH v5 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.

The read is caught by KASAN under a KUnit fault-injection harness; I have
no Brocade hardware, so the frame and its oversized payload length are
injected, with the arithmetic and the loop bounds modelled against an
unsolicited frame buffer:

  BUG: KASAN: slab-out-of-bounds in bfa_fcs_lport_scn_process_rscn+0x1156/0x1750
  Read of size 4 at addr 0000000062270900 by task kunit_try_catch/19

  Call Trace:
   [<60935866>] ? bfa_fcs_lport_scn_process_rscn+0x1156/0x1750
   [<603b5d8b>] ? kasan_report+0x11b/0x160

  The buggy address belongs to the object at 0000000062270000
   which belongs to the cache kmalloc-4k of size 4096
  The buggy address is located 0 bytes to the right of
   allocated 2304-byte region [0000000062270000, 0000000062270900)

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

Re: [PATCH v5 2/3] scsi: bfa: drop unsolicited frames shorter than the headers they must hold

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] scsi: bfa: OOB read of ELS command code due to missing payload bounds check
--

commit 31f50cb105260d95a224555b9e6f894b5a59692b
Author: Yehyeong Lee <yhlee@isslab.korea.ac.kr>

scsi: bfa: drop unsolicited frames shorter than the headers they must hold

bfa_fcs_uf_recv() keeps the received frame length in a u16 and hands it,
with a pointer to the frame, to every unsolicited frame consumer. Two
lengths are never checked.

> diff --git a/drivers/scsi/bfa/bfa_fcs.c b/drivers/scsi/bfa/bfa_fcs.c
> index 9b57312f43f50..8bb92f935a40f 100644
> --- a/drivers/scsi/bfa/bfa_fcs.c
> +++ b/drivers/scsi/bfa/bfa_fcs.c
> @@ -1527,6 +1527,14 @@ bfa_fcs_uf_recv(void *cbarg, struct bfa_uf_s *uf)
>  	struct fc_vft_s *vft;
>  	struct bfa_fcs_fabric_s *fabric;
>  
> +	/*
> +	 * every consumer reads the FC header, so the frame has to hold one
> +	 */
> +	if (len < sizeof(struct fchs_s)) {
> +		bfa_uf_free(uf);
> +		return;
> +	}
> +

[Severity: High]
Does this check need to account for the payload length when processing ELS
commands?

If a remote device sends an unsolicited FC frame of exactly 24 bytes with
fchs->type == FC_TYPE_ELS, can this cause a regression where the driver reads
beyond the verified length?

For example, in drivers/scsi/bfa/bfa_fcs.c:bfa_fcs_fabric_uf_recv():

    struct fc_els_cmd_s *els_cmd = (struct fc_els_cmd_s *) (fchs + 1);
    ...
    if ((pid == bfa_ntoh3b(FC_FABRIC_PORT)) &&
        (els_cmd->els_code == FC_ELS_FLOGI) && ...

And similarly in drivers/scsi/bfa/bfa_fcs_lport.c:bfa_fcs_lport_uf_recv():

    struct fc_els_cmd_s *els_cmd = (struct fc_els_cmd_s *) (fchs + 1);
    ...
    if ((fchs->type == FC_TYPE_ELS) &&
        (els_cmd->els_code == FC_ELS_PLOGI)) {

Because the driver recycles DMA buffers without zeroing them, the hardware DMA
writes only 24 bytes, leaving the old payload intact in the buffer. Could the
driver read the stale els_code and process the stale payload as a new command?

Should there be an additional length verification before accessing
els_cmd->els_code?

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

Re: [PATCH v5 2/3] scsi: bfa: drop unsolicited frames shorter than the headers they must hold

[Yehyeong Lee]

On the 24-byte case: a frame of exactly sizeof(struct fchs_s) passes the new
guard, and the dispatch then reads els_cmd->els_code at offset 24. For such a
frame that byte is past the frame, but it is still inside the unsolicited-frame
buffer, which is a fixed sizeof(struct bfa_uf_buf_s) that claim_uf_post_msgs()
posts and patch 1 clamps the length to -- so this is a stale in-buffer read,
not an out-of-bounds access. The two handlers that use the length,
bfa_fcs_lport_echo() and bfa_fcs_lport_scn_process_rscn(), are both bounded at
24 bytes (pyld_len == 0, so the memcpy does not run; max_entries == 0, so the
walk does not run).

The minimum cannot be raised to also cover the ELS command word, because a
24-byte header-only frame is legitimate: ABTS is a Basic Link Service frame
with no payload and is dispatched from bfa_fcs_lport_uf_recv() at exactly
sizeof(struct fchs_s). Requiring more would drop valid ABTS frames.

Checking each ELS/BLS against its own minimum payload length is a separate
change; this series bounds the frame length itself -- the FC header below, the
posted buffer above -- and the two handlers that consume it.

Yehyeong