Re: [PATCH 1/2] scsi: lpfc: bound EDC descriptor list by payload length

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [Critical] An integer overflow in the FC_TLV_DESC_SZ_FROM_LENGTH macro allows an attacker to bypass bounds checks, resulting in kernel panics or infinite loops.
- [High] Missing bounds check on unknown TLV descriptors in lpfc_els_rcv_edc() and lpfc_cmpl_els_edc() allows KASAN out-of-bounds read.
--

--- Patch [1]: [PATCH 1/2] scsi: lpfc: bound EDC descriptor list by payload length ---
commit 12e45848685e8826e33fd1952148968b98cdc126
Author: Michael Bommarito <michael.bommarito@gmail.com>

    scsi: lpfc: bound EDC descriptor list by payload length
    
    drivers/scsi/lpfc/lpfc_els.c:lpfc_els_rcv_edc() trusts the EDC
    descriptor-list length from the received frame without checking that it
    fits in the actual ELS payload. An adjacent Fibre Channel fabric peer or
    device can send an unsolicited EDC frame with a short payload and an
    oversized descriptor-list length. The TLV walk can then read past the
    receive buffer and trip a KASAN slab-out-of-bounds read in the LPFC ELS
    receive path.

> diff --git a/drivers/scsi/lpfc/lpfc_els.c b/drivers/scsi/lpfc/lpfc_els.c
> index 52fc5058976d9..0023524528d93 100644
> --- a/drivers/scsi/lpfc/lpfc_els.c
> +++ b/drivers/scsi/lpfc/lpfc_els.c
[ ... ]
> @@ -9443,25 +9444,39 @@ lpfc_els_rcv_edc(struct lpfc_vport *vport, struct lpfc_iocbq *cmdiocb,
>  	uint8_t *payload;
>  	uint32_t *ptr, dtag;
>  	const char *dtag_nm;
> -	int desc_cnt = 0, bytes_remain;
> +	int desc_cnt = 0;
> +	u32 bytes_remain, desc_len, word2 = 0;

[Severity: High]
This is a pre-existing issue, but does changing bytes_remain to u32 bypass
the while loop condition if we encounter an unknown TLV descriptor?

If an adjacent peer sends an EDC frame with an unrecognized descriptor tag,
we hit the default case in lpfc_els_rcv_edc() (and lpfc_cmpl_els_edc()),
where we subtract the unvalidated parsed TLV length from bytes_remain:

drivers/scsi/lpfc/lpfc_els.c:lpfc_els_rcv_edc() {
    ...
		default:
			dtag_nm = lpfc_get_tlv_dtag_nm(dtag);
			...
		}
		bytes_remain -= FC_TLV_DESC_SZ_FROM_LENGTH(tlv);
		tlv = fc_tlv_next_desc(tlv);
    ...
}

Since bytes_remain is now u32, subtracting an unvalidated length could
underflow to a large positive value. This would bypass the 
bytes_remain < FC_TLV_DESC_HDR_SZ check on the next iteration and lead to
an out-of-bounds memory read.

[ ... ]
> @@ -9491,7 +9506,7 @@ lpfc_els_rcv_edc(struct lpfc_vport *vport, struct lpfc_iocbq *cmdiocb,
>  				lpfc_printf_log(phba, KERN_WARNING,
>  					LOG_ELS | LOG_CGN_MGMT | LOG_LDS_EVENT,
>  					"6465 Truncated Link Fault Diagnostic "
> -					"descriptor[%d]: %d vs 0x%zx 0x%zx\n",
> +					"descriptor[%d]: %u vs 0x%zx 0x%zx\n",
>  					desc_cnt, bytes_remain,
>  					FC_TLV_DESC_SZ_FROM_LENGTH(tlv),

[Severity: Critical]
This isn't a bug introduced by this patch, but can FC_TLV_DESC_SZ_FROM_LENGTH
overflow and bypass bounds checks entirely?

Looking at the macro definition:

include/uapi/scsi/fc/fc_els.h:
#define FC_TLV_DESC_SZ_FROM_LENGTH(tlv)		\
		(__be32_to_cpu((tlv)->desc_len) + FC_TLV_DESC_HDR_SZ)

It performs an unchecked addition of 8 to an attacker-controlled 32-bit
integer. If an attacker crafts a desc_len near the 32-bit maximum limit
(e.g., 0xFFFFFFF8), the macro could overflow to exactly 0 on 32-bit systems,
or 4GB on 64-bit systems.

In lpfc_els_rcv_fpin(), an overflow to 0 allows this bounds check to pass
incorrectly:

drivers/scsi/lpfc/lpfc_els.c:lpfc_els_rcv_fpin() {
    ...
	while (bytes_remain >= FC_TLV_DESC_HDR_SZ &&
	       bytes_remain >= FC_TLV_DESC_SZ_FROM_LENGTH(tlv)) {
    ...
}

Since the evaluated length is 0, neither bytes_remain nor the tlv pointer
would advance, causing an infinite loop. On 64-bit systems, this could lead
to a panic when the large offset is applied to the pointer.

>  					sizeof(struct fc_diag_lnkflt_desc));

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260710022932.3741311-1-michael.bommarito@gmail.com?part=1