[PATCH] scsi: bnx2fc: drop unsolicited frames shorter than an FC header

[Yehyeong Lee]

bnx2fc_process_l2_frame_compl() works out the payload size of an
unsolicited frame by subtracting the FC header from the length it was
handed:

	payload_len = frame_len - sizeof(struct fc_frame_header);

	fp = fc_frame_alloc(lport, payload_len);
	if (!fp) {

On the unsolicited completion path that length comes from the chip.
bnx2fc_process_unsol_compl() takes it out of the work queue entry with
FCOE_UNSOLICITED_CQE_PKT_LEN, a twelve bit field, and nothing requires it
to be at least an FC header.  Below that the subtraction underflows and
payload_len ends up close to 4 GiB.

fc_frame_alloc() does not refuse that.  When payload_len is not a multiple
of four it goes by way of fc_frame_alloc_fill(), which hands payload_len +
fill to _fc_frame_alloc(), where alloc_skb_fclone() and skb_put() both take
an unsigned int and quietly truncate it.  The allocation then succeeds at a
few dozen bytes while payload_len is still 0xffffffff, and the

	memset((char *) fr_hdr(fp) + payload_len, 0, fill);

that follows writes a byte roughly 4 GiB past the frame.  A frame_len of 23
is enough.  The harness below is a KUnit test that is not part of this
patch; it calls the real function with the frame buffer kmalloc()ed at the
length under test, and fc_frame_alloc_fill() and
bnx2fc_process_l2_frame_compl() are the two frames above memset_orig in the
trace, with payload_len still in RBX:

  RIP: 0033:memset_orig+0x81/0xaf
  RSP: 0000000062893df8  EFLAGS: 00010246
  RAX: 0000000000000000 RBX: 00000000ffffffff RCX: 0000000000000000
  RDX: 0000000000000000 RSI: 0000000000000000 RDI: 00000001628a779f
  RBP: 0000000062896e00 R08: 0000000000000001 R09: 0000000000000007
  R10: 00000001628a779f R11: 0000000000000004 R12: 0000000000000001
  R13: 00000000628b4000 R14: 0000000062887200 R15: 000000006272ba00
  Kernel panic - not syncing: Segfault with no mm

A length below the header that is a multiple of four does not panic, but it
is not handled either.  _fc_frame_alloc() truncates the same way, the frame
comes back with an fr_len() of four, and fc_frame_header_get() trips its
WARN_ON() in fc_frame.h before the handler copies anything into it.  A
frame_len of 20 produces that warning.

Nothing here needs an upper bound.  The unsolicited path sizes the buffer
it passes down from frame_len itself, rounded up to whole RQ entries, and
bnx2fc_get_next_rqe() refuses a run that would wrap the ring, so the buffer
always holds at least frame_len bytes.  Only the lower end is unchecked.

Drop such a frame.  The other caller, the one in bnx2fc_l2_els_compl(),
builds frame_len as sizeof(*fc_hdr) + resp_len and so can never be short,
which leaves the check a no-op there.

With it in place the panic and the warning both go, while a frame carrying
an ELS request and one at an ordinary size are still allocated, copied and
passed on.

Fixes: 853e2bd2103a ("[SCSI] bnx2fc: Broadcom FCoE offload driver")
Cc: stable@xxxxxxxxxxxxxxx
Signed-off-by: Yehyeong Lee <yhlee@xxxxxxxxxxxxxxxxxx>
---
 drivers/scsi/bnx2fc/bnx2fc_hwi.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/scsi/bnx2fc/bnx2fc_hwi.c b/drivers/scsi/bnx2fc/bnx2fc_hwi.c
index a5ecb87d5b2d7..fc6396ed00f62 100644
--- a/drivers/scsi/bnx2fc/bnx2fc_hwi.c
+++ b/drivers/scsi/bnx2fc/bnx2fc_hwi.c
@@ -570,6 +570,14 @@ void bnx2fc_process_l2_frame_compl(struct bnx2fc_rport *tgt,
 	BNX2FC_TGT_DBG(tgt, "l2_frame_compl l2_oxid = 0x%x, frame_len = %d\n",
 		l2_oxid, frame_len);
 
+	/* Anything shorter underflows the subtraction below. */
+	if (frame_len < sizeof(struct fc_frame_header)) {
+		BNX2FC_TGT_DBG(tgt, "dropping short frame, frame_len = %d\n",
+			       frame_len);
+		kfree(unsol_els);
+		return;
+	}
+
 	payload_len = frame_len - sizeof(struct fc_frame_header);
 
 	fp = fc_frame_alloc(lport, payload_len);

base-commit: c03c1a14507dfaf8b856c2608f9ac216021fc222
-- 
2.43.0