Re: [PATCH 2/2] scsi: lpfc: add KUnit coverage for EDC descriptor bounds

[sashiko-bot@kernel.org]

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

New issues:
- [Medium] The KUnit test leaks memory pools if an assertion fails because cleanup is deferred instead of registered with KUnit.
- [Medium] The KUnit test uses an uninitialized `ndlp` node, causing the test to silently abort early and mask a missing mock function crash.

Pre-existing issues:
- [High] Missing bounds check for unknown EDC diagnostic descriptors allows an underflow of `bytes_remain`, leading to an out-of-bounds memory read or infinite loop.
--

--- Patch [2]: [PATCH 2/2] scsi: lpfc: add KUnit coverage for EDC descriptor bounds ---
commit dd6d0b3cfc06c72b1e47a15c2af79c7992a6552f
Author: Michael Bommarito <michael.bommarito@gmail.com>

scsi: lpfc: add KUnit coverage for EDC descriptor bounds

This patch adds KUnit coverage for lpfc_els_rcv_edc() descriptor-list bounds.
The tests live in lpfc_els.c so they can drive the real static EDC parser
without exporting test-only symbols.

> diff --git a/drivers/scsi/lpfc/lpfc_els.c b/drivers/scsi/lpfc/lpfc_els.c
> index 0023524528d93..3193a2aaac2f1 100644
> --- a/drivers/scsi/lpfc/lpfc_els.c
> +++ b/drivers/scsi/lpfc/lpfc_els.c
> @@ -12592,3 +12592,157 @@ lpfc_cmpl_els_uvem(struct lpfc_hba *phba, struct lpfc_iocbq *icmdiocb,

[ ... ]

> +static void lpfc_edc_kunit_setup_hba(struct kunit *test,
> +				     struct lpfc_hba *phba,
> +				     struct lpfc_vport *vport)
> +{

[ ... ]

> +	phba->lpfc_mbuf_pool = dma_pool_create("lpfc_edc_mbuf", kdev,
> +					       LPFC_BPL_SIZE, 8, 0);
> +	KUNIT_ASSERT_NOT_NULL(test, phba->lpfc_mbuf_pool);
> +	phba->mbox_mem_pool = mempool_create_kmalloc_pool(1,
> +							  sizeof(LPFC_MBOXQ_t));
> +	KUNIT_ASSERT_NOT_NULL(test, phba->mbox_mem_pool);
> +}

[Severity: Medium]
If any KUnit assertion fails later in lpfc_edc_kunit_run() (such as the
allocation check for cmd_dmabuf->virt), the test will abort immediately 
without executing lpfc_edc_kunit_teardown_hba(). Will this leak the dma
and memory pools created here? Should these be registered for automatic
cleanup with KUnit instead?

[ ... ]

> +static void lpfc_edc_kunit_run(struct kunit *test, bool malformed)
> +{
> +	struct lpfc_hba *phba;
> +	struct lpfc_vport *vport;
> +	struct lpfc_nodelist *ndlp;
> +	struct lpfc_iocbq *cmdiocb;
> +	struct lpfc_dmabuf *cmd_dmabuf;
> +	struct fc_els_edc *edc;
> +	struct fc_tlv_desc *tlv;
> +	size_t payload_len;
> +
> +	phba = kunit_kzalloc(test, sizeof(*phba), GFP_KERNEL);
> +	vport = kunit_kzalloc(test, sizeof(*vport), GFP_KERNEL);
> +	ndlp = kunit_kzalloc(test, sizeof(*ndlp), GFP_KERNEL);

[Severity: Medium]
Is the kref for this node list left as zero by the zero-allocation?

When lpfc_els_rcv_edc() calls lpfc_issue_els_edc_rsp() later in this path,
it attempts to take a reference via lpfc_nlp_get(ndlp). Because kref is
zero, won't this fail and return NULL, causing the function to silently abort
and skip sending the response? 

This early abort would mask a potential crash, as phba->__lpfc_sli_issue_iocb
does not appear to be stubbed in the lpfc_edc_kunit_setup_hba() mock setup.

> +	cmdiocb = kunit_kzalloc(test, sizeof(*cmdiocb), GFP_KERNEL);
> +	cmd_dmabuf = kunit_kzalloc(test, sizeof(*cmd_dmabuf), GFP_KERNEL);
> +	KUNIT_ASSERT_NOT_NULL(test, phba);
> +	KUNIT_ASSERT_NOT_NULL(test, vport);
> +	KUNIT_ASSERT_NOT_NULL(test, ndlp);
> +	KUNIT_ASSERT_NOT_NULL(test, cmdiocb);
> +	KUNIT_ASSERT_NOT_NULL(test, cmd_dmabuf);

[ ... ]

> +	cmdiocb->cmd_dmabuf = cmd_dmabuf;
> +	lpfc_els_rcv_edc(vport, cmdiocb, ndlp, payload_len);

[Severity: High]
This is a pre-existing issue, but missing bounds checks for unknown EDC
diagnostic descriptors in lpfc_els_rcv_edc() could allow an underflow of
bytes_remain.

In the target function:

lpfc_els.c:lpfc_els_rcv_edc() {
    ...
    while (bytes_remain) {
        ...
        switch (dtag) {
        ...
        default:
            dtag_nm = lpfc_get_tlv_dtag_nm(dtag);
            lpfc_printf_log(phba, KERN_WARNING, ...);
        }
        bytes_remain -= FC_TLV_DESC_SZ_FROM_LENGTH(tlv);
        tlv = fc_tlv_next_desc(tlv);
    ...
}

If a remote FC node sends an unknown descriptor tag whose length is maliciously
crafted to be larger than the remaining payload, won't bytes_remain underflow
to a huge value since there is no bounds check in the default switch case?
This could bypass the loop termination and allow an out-of-bounds read on the
next iteration.

> +
> +	KUNIT_EXPECT_TRUE(test, true);
> +	lpfc_edc_kunit_teardown_hba(phba);
> +}

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