Re: [PATCH v3 01/14] lpfc: Fix use-after-free in lpfc_cmpl_ct_cmd_vmid

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [Critical] Zeroing hash table elements before their removal causes a guaranteed kernel panic, and unsafe hash table iteration breaks list cleanup.
- [High] Multiple severe locking violations in the VMID hash table and array accesses, including using a read lock for destructive writes and lockless modifications.
- [High] The SLI_CTAS_DALLAPP_ID handler leaks per-cpu memory by memsetting the lpfc_vmid structure to zero without freeing `last_io_time`.
- [High] In `lpfc_vmid_vport_cleanup`, `vport->vmid` is freed via `kfree` before the code iterates over `vport->hash_table` to remove entries, resulting in a Use-After-Free.
--

--- Patch [1]: [PATCH v3 01/14] lpfc: Fix use-after-free in lpfc_cmpl_ct_cmd_vmid ---
commit ade52a05f937db7fd95e3ae568536fda43aeccdf
Author: Justin Tee <justintee8345@gmail.com>

lpfc: Fix use-after-free in lpfc_cmpl_ct_cmd_vmid

This commit removes an early call to lpfc_ct_free_iocb() in
lpfc_cmpl_ct_cmd_vmid() for SLI_CTAS_DALLAPP_ID commands. This prevents a
use-after-free of cmdiocb->rsp_dmabuf during CT LS_RJT response handling,
relying instead on the existing free_res label for cleanup.

> diff --git a/drivers/scsi/lpfc/lpfc_ct.c b/drivers/scsi/lpfc/lpfc_ct.c
> index c7853e7fe0719..e14170550e697 100644
> --- a/drivers/scsi/lpfc/lpfc_ct.c
> +++ b/drivers/scsi/lpfc/lpfc_ct.c
> @@ -3595,8 +3595,6 @@ lpfc_cmpl_ct_cmd_vmid(struct lpfc_hba *phba, struct lpfc_iocbq *cmdiocb,
>  	int i;
>  
>  	cmd = be16_to_cpu(ctcmd->CommandResponse.bits.CmdRsp);
> -	if (cmd == SLI_CTAS_DALLAPP_ID)
> -		lpfc_ct_free_iocb(phba, cmdiocb);
>  
>  	if (lpfc_els_chk_latt(vport) || get_job_ulpstatus(phba, rspiocb)) {
>  		if (cmd != SLI_CTAS_DALLAPP_ID)

[Severity: Critical]
This is a pre-existing issue, but looking further down in
lpfc_cmpl_ct_cmd_vmid() under the SLI_CTAS_DALLAPP_ID case:

    memset(vmp, 0, sizeof(struct lpfc_vmid));

This zeroes the embedded hlist_node, including its pprev and next pointers.
Right after this, the code attempts to remove these items:

    if (!hash_empty(vport->hash_table))
        hash_for_each(vport->hash_table, bucket, cur, hnode)
            hash_del(&cur->hnode);

Does calling hash_del() on these zeroed elements cause a NULL pointer
dereference panic when it tries to write to *n->pprev?

Also, since this iteration modifies the list, shouldn't it use
hash_for_each_safe() to prevent breaking the iteration chain?

[Severity: High]
This isn't a bug introduced by this patch, but there appear to be multiple
locking violations in lpfc_cmpl_ct_cmd_vmid() that could lead to data races
and corruption.

First, under the SLI_CTAS_DALLAPP_ID case, the memset() is performed under a
read lock:

    read_lock(&vport->vmid_lock);
    for (i = 0; i < phba->cfg_max_vmid; i++) {
        vmp = &vport->vmid[i];
        if (vmp->flag != LPFC_VMID_SLOT_FREE)
            memset(vmp, 0, sizeof(struct lpfc_vmid));
    }
    read_unlock(&vport->vmid_lock);

Since read_lock() doesn't provide mutual exclusion against concurrent readers,
does this destructive write race with other readers?

Additionally, the hash table iteration and deletion:

    if (!hash_empty(vport->hash_table))
        hash_for_each(vport->hash_table, bucket, cur, hnode)
            hash_del(&cur->hnode);

is performed entirely locklessly.

Furthermore, earlier in the SLI_CTAS_RAPP_IDENT case:

    vmp = lpfc_get_vmid_from_hashtable(vport, hash,
                                       app->obj.entity_id);

This lookup is also performed without holding any lock.

Can these lockless operations and the write-under-read-lock cause list
corruption?

[Severity: High]
This is a pre-existing issue, but the same memset() in lpfc_cmpl_ct_cmd_vmid()
that zeroes the hlist_node:

    memset(vmp, 0, sizeof(struct lpfc_vmid));

also overwrites the entire lpfc_vmid struct. This struct contains a dynamically
allocated per-cpu pointer:

    u64 __percpu *last_io_time;

Does this zeroing cause an unrecoverable leak of the per-cpu memory allocated
for last_io_time?

[Severity: High]
This is also a pre-existing issue, but related to the VMID hash table cleanup
seen above. In drivers/scsi/lpfc/lpfc_scsi.c:lpfc_vmid_vport_cleanup():

    kfree(vport->vmid);

    if (!hash_empty(vport->hash_table))
        hash_for_each(vport->hash_table, bucket, cur, hnode)
            hash_del(&cur->hnode);

The hash table nodes (hnode) are embedded within the lpfc_vmid structures in
the vport->vmid array.

Since kfree(vport->vmid) is called before iterating the hash table, doesn't
the subsequent hash_for_each() loop read from freed memory?

Furthermore, shouldn't hash_for_each_safe() be used here as well since
hash_del() modifies the list?

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