qedf_process_unsol_compl() takes the length of an unsolicited FCoE frame out of the completion queue entry and copies that many bytes out of the BDQ buffer the firmware put the frame in: uint16_t pktlen = cqe->cqe_info.unsolic_info.pkt_len; ... bdq_addr = qedf->bdq[bdq_idx].buf_addr; if (!bdq_addr) { ... memcpy(fh, (void *)bdq_addr, pktlen); The BDQ index and a NULL buffer are both checked; the length is not. A BDQ buffer is a QEDF_BDQ_BUF_SIZE coherent DMA allocation and pkt_len is a __le16, so a frame the firmware reports as longer than 2072 bytes reads up to 63 KiB past the end of one. The print_hex_dump() under qedf_dump_frames a few lines earlier reads the same length out of the same buffer. The destination is not where this goes wrong. fc_frame_alloc() is handed pktlen - sizeof(struct fc_frame_header) and sets the frame length to that plus the header again, so there is room for exactly pktlen bytes from fc_frame_header_get() onwards. Only the source runs off the end. How much room there is was the driver's decision in the first place: it advertises QEDF_BDQ_BUF_SIZE to the firmware as rq_buffer_size while setting up its PF parameters, and allocates every BDQ buffer at exactly that size. A pkt_len above it contradicts the size the driver itself handed over, so there is nothing to be gained by trying to honour it. A lower bound belongs with the upper one, and not merely for tidiness. payload_len underflows for any length below an FC header, and fc_frame_alloc() then takes the fc_frame_alloc_fill() path whenever the result is not a multiple of four. That passes payload_len + fill on to _fc_frame_alloc(), where alloc_skb_fclone() and skb_put() both take an unsigned int and quietly truncate it, so the allocation succeeds at a few dozen bytes while payload_len stays at 0xffffffff. The memset((char *) fr_hdr(fp) + payload_len, 0, fill); that follows then writes a byte roughly 4 GiB past the frame. Drop such a frame rather than clamping it. A truncated FC frame is not something libfc can do anything useful with, and the out-of-range cases this handler already has take the increment_prod exit so that the producer index still advances and the BDQ entry goes back to the firmware. Driving the handler with a reported length one byte past a BDQ buffer shows the copy running off the source. The harness is a KUnit test that is not part of this patch; it calls the real handler with the BDQ buffer swapped for kmalloc() of the same size, because coherent DMA allocations are page granular and KASAN does not track them: BUG: KASAN: slab-out-of-bounds in qedf_process_unsol_compl+0x336/0x10b0 Read of size 2073 at addr 00000000627ca000 by task kunit_try_catch/25 CPU: 0 UID: 0 PID: 25 Comm: kunit_try_catch Tainted: G N 7.3.0-rc7-g58161ead29e5-dirty #3 VOLUNTARY Tainted: [N]=TEST Stack: 6133bca2 00000000 00000000 00000000 ffffff00 6133bca2 61343506 627ca000 613d1680 600c3c61 62893d00 6004301e Call Trace: [<600c3c61>] ? dump_stack_lvl+0x5e/0x7a [<6004301e>] ? _printk+0x0/0xd7 [<6004fc1a>] ? print_report+0x14b/0x517 [<603b68e4>] ? __kmalloc_node_track_caller_noprof+0x1a4/0x3c0 [<60ada41d>] ? __alloc_skb+0x1bd/0x9c0 [<603b6740>] ? __kmalloc_node_track_caller_noprof+0x0/0x3c0 [<6004301e>] ? _printk+0x0/0xd7 [<609437f6>] ? qedf_process_unsol_compl+0x336/0x10b0 [<603e0d0b>] ? kasan_report+0x11b/0x160 [<609437f6>] ? qedf_process_unsol_compl+0x336/0x10b0 [<60932a10>] ? qedf_test_pool_alloc+0x0/0x30 [<603e1cc0>] ? kasan_check_range+0x0/0x1c0 [<603e1e0e>] ? kasan_check_range+0x14e/0x1c0 [<603e2891>] ? __asan_memcpy+0x31/0x80 [<609437f6>] ? qedf_process_unsol_compl+0x336/0x10b0 [<600e147d>] ? os_nsecs+0x2d/0x40 [<60944ec1>] ? qedf_unsol_oob_2073+0xc1/0x260 [<602453a0>] ? ktime_get_ts64+0x0/0x2c0 [<6071e2ac>] ? kunit_try_run_case+0x1bc/0x310 [<6071e0f0>] ? kunit_try_run_case+0x0/0x310 [<601d9b30>] ? sched_setscheduler_nocheck+0x180/0x220 [<600e0e70>] ? um_set_signals+0x0/0x60 [<600e0e70>] ? um_set_signals+0x0/0x60 [<6016021d>] ? __kthread_parkme+0xdd/0x1c0 [<6071e0f0>] ? kunit_try_run_case+0x0/0x310 [<60724370>] ? kunit_generic_run_threadfn_adapter+0x0/0x100 [<607243ec>] ? kunit_generic_run_threadfn_adapter+0x7c/0x100 [<6016649e>] ? kthread+0x36e/0x560 [<60166130>] ? kthread+0x0/0x560 [<600c80de>] ? new_thread_handler+0xce/0x130 The buggy address belongs to the object at 00000000627ca000 which belongs to the cache kmalloc-4k of size 4096 The buggy address is located 0 bytes inside of allocated 2072-byte region [00000000627ca000, 00000000627ca818) The buggy address belongs to the physical page: page: refcount:0 mapcount:0 mapping:0000000000000000 index:0x0 pfn:0x27c8 head: order:3 mapcount:0 entire_mapcount:0 nr_pages_mapped:0 pincount:0 flags: 0x40(head|zone=0) page_type: f5(slab) raw: 0000000000000040 0000000061802140 0000000000000122 0000000000000000 raw: 0000000000000000 0000000000040004 00000000f5000000 head: 0000000000000040 0000000061802140 0000000000000122 0000000000000000 head: 0000000000000000 0000000000040004 00000000f5000000 0000000000000003 head: 000000009f28b3c1 00000000ffffffff 00000000ffffffff 0000000000000000 head: 0000000000000000 00000000ffffffff page dumped because: kasan: bad access detected Memory state around the buggy address: 00000000627ca700: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00000000627ca780: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 >00000000627ca800: 00 00 00 fc fc fc fc fc fc fc fc fc fc fc fc fc ^ 00000000627ca880: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc 00000000627ca900: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc ================================================================== Reported lengths of 4097 and 65535 report the same way at their own sizes. A reported length of 23 takes the kernel down instead, with fc_frame_alloc_fill() and qedf_process_unsol_compl() the two frames above memset_orig in the trace and payload_len still in RBX: RIP: 0033:memset_orig+0x81/0xaf RSP: 00000000628c3d68 EFLAGS: 00010246 RAX: 0000000000000000 RBX: 00000000ffffffff RCX: 0000000000000000 RDX: 0000000000000000 RSI: 0000000000000000 RDI: 0000000161a2439f RBP: 00000000628a2c00 R08: 0000000000000001 R09: 0000000000000007 R10: 0000000161a2439f R11: 00000000609128c0 R12: 0000000000000001 R13: 0000000062740608 R14: 00000000611f1d40 R15: 00000000628c8000 Kernel panic - not syncing: Segfault with no mm With the length checked, the three over-reads and the panic all go away, while a frame that fills a BDQ buffer exactly and an ordinary small one are both still handed to libfc. Fixes: 61d8658b4a43 ("scsi: qedf: Add QLogic FastLinQ offload FCoE driver framework.") Cc: stable@vger.kernel.org Signed-off-by: Yehyeong Lee --- diff --git a/drivers/scsi/qedf/qedf_io.c b/drivers/scsi/qedf/qedf_io.c index a120f0e37a644..807202f575499 100644 --- a/drivers/scsi/qedf/qedf_io.c +++ b/drivers/scsi/qedf/qedf_io.c @@ -2504,6 +2504,15 @@ void qedf_process_unsol_compl(struct qedf_ctx *qedf, uint16_t que_idx, le32_to_cpu(p_bd_info->opaque.fcoe_opaque.lo), qedf->bdq_prod_idx, pktlen); + /* The firmware can only have filled the buffer it was given. */ + if (pktlen < sizeof(struct fc_frame_header) || + pktlen > QEDF_BDQ_BUF_SIZE) { + QEDF_ERR(&qedf->dbg_ctx, + "pkt_len %u out of range, dropping unsolicited frame.\n", + pktlen); + goto increment_prod; + } + bdq_idx = le32_to_cpu(p_bd_info->opaque.fcoe_opaque.lo); if (bdq_idx >= QEDF_BDQ_SIZE) { QEDF_ERR(&(qedf->dbg_ctx), "bdq_idx is out of range %d.\n",