Re: [PATCH 1/2] hisi_acc_vfio_pci: fix live migration enable conditions for PF passthrough

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

Pre-existing issues:
- [High] The patch fails to prevent a Use-After-Free because `pf_qm` becomes a dangling pointer (not NULL) when the PF driver is unbound.
- [High] `set_reset_flag` is never cleared, causing spurious lock release during subsequent failed resets.
--

--- Patch [1]: [PATCH 1/2] hisi_acc_vfio_pci: fix live migration enable conditions for PF passthrough ---
commit 2cccd4c0444989d0fd451c1227c40bde4c25acb2
Author: Longfang Liu <[email protected]>
Subject: hisi_acc_vfio_pci: fix live migration enable conditions for PF passthrough

This commit attempts to fix a calltrace exception triggered when a user
unbinds the PF device driver from the host and directly passes it through to
a VM, then attempts live migration. It adds checks across operations that
depend on PF driver commands to ensure the PF's QM structure is available.

> diff --git a/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.c b/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.c
> index 86362ec424a50..36490be7a61aa 100644
> --- a/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.c
> +++ b/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.c
> @@ -378,6 +378,11 @@ static int vf_qm_check_match(struct hisi_acc_vf_core_device *hisi_acc_vdev,
>  	if (migf->total_length < QM_MATCH_SIZE || hisi_acc_vdev->match_done)
>  		return 0;
>  
> +	if (!pf_qm || !pf_qm->io_base) {

[Severity: High]
This isn't a bug introduced by this patch, but since the patch attempts to
fix it here, does this check fully prevent a Use-After-Free?

When the PF driver is unbound while a VF is assigned to a VM,
pci_disable_sriov() warns but doesn't prevent the driver's remove callback
from executing, causing the PF's qm structure to be freed via devres.

This leaves hisi_acc_vdev->pf_qm as a dangling pointer rather than NULL.
Will the !pf_qm check fail to catch this, leading to a UAF read when
accessing pf_qm->io_base?

> +		dev_err(dev, "failed to match check for PF QM migration\n");
> +		return -ENODEV;
> +	}
> +
>  	ret = vf_qm_version_check(vf_data, dev);

[ ... ]

> @@ -1174,8 +1194,12 @@ static void hisi_acc_vf_pci_aer_reset_done(struct pci_dev *pdev)
>  	struct hisi_acc_vf_core_device *hisi_acc_vdev = hisi_acc_drvdata(pdev);
>  	struct hisi_qm *qm = hisi_acc_vdev->pf_qm;
>  
> -	if (hisi_acc_vdev->set_reset_flag)
> -		clear_bit(QM_RESETTING, &qm->misc_ctl);
> +	if (hisi_acc_vdev->set_reset_flag) {
> +		if (qm && qm->io_base)
> +			clear_bit(QM_RESETTING, &qm->misc_ctl);

[Severity: High]
This is a pre-existing issue, but should the set_reset_flag be cleared after
releasing this lock?

If hisi_acc_vf_pci_reset_prepare() successfully acquires the lock, it sets
set_reset_flag = true. However, when hisi_acc_vf_pci_aer_reset_done()
clears the lock here, it doesn't appear to reset the flag back to false.

If a subsequent reset occurs and reset_prepare() fails to acquire the lock
(e.g., due to a timeout because another VF or the PF holds it), won't this
function erroneously clear the QM_RESETTING lock that is actively held by
another context?

> +		else
> +			dev_err(&pdev->dev, "PF QM not available for reset done\n");
> +	}
>  
>  	if (!hisi_acc_vdev->core_device.vdev.mig_ops)
>  		return;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1