Re: [PATCH v4 03/16] vfio/pci: Retrieve preserved device files after Live Update

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] vfio_pci_liveupdate_can_finish logic error permanently leaks preserved KHO memory and wedges the session.
- [High] TOCTOU race in vfio_pci_liveupdate_freeze allows device reset while interrupts are active.
--

commit aeb30af9099697f73a40eb35a514bc4eaae76916
Author: Vipin Sharma <vipinsh@google.com>

vfio/pci: Retrieve preserved device files after Live Update

This commit implements the retrieve() and finish() file handler callbacks for
VFIO, enabling userspace to retrieve preserved VFIO device files after a Live
Update.

> diff --git a/drivers/vfio/pci/vfio_pci_liveupdate.c b/drivers/vfio/pci/vfio_pci_liveupdate.c
> index 5cc57bf39dbee..9142aaea94f2d 100644
> --- a/drivers/vfio/pci/vfio_pci_liveupdate.c
> +++ b/drivers/vfio/pci/vfio_pci_liveupdate.c

[ ... ]

> @@ -203,13 +210,53 @@ static int vfio_pci_liveupdate_freeze(struct liveupdate_file_op_args *args)

[ ... ]

> +static bool vfio_pci_liveupdate_can_finish(struct liveupdate_file_op_args *args)
> +{
> +	return args->retrieve_status > 0;
> +}

Will this permanently leak preserved KHO memory and wedge the session if
retrieval fails or is never attempted?

If the Live Update Orchestrator (LUO) sets retrieve_status to 0 (userspace
never attempted retrieval) or to a negative error code (retrieval failed),
can_finish() will evaluate to false.

This will cause the LUO core's luo_file_finish() routine to abort with -EBUSY,
bypassing the finish() callback and leaking the KHO memory allocated for
args->serialized_data.

>  static void vfio_pci_liveupdate_finish(struct liveupdate_file_op_args *args)
>  {
> +	kho_restore_free(phys_to_virt(args->serialized_data));
>  }
>  
>  static const struct liveupdate_file_ops vfio_pci_liveupdate_file_ops = {
> @@ -218,6 +265,7 @@ static const struct liveupdate_file_ops vfio_pci_liveupdate_file_ops = {
>  	.unpreserve = vfio_pci_liveupdate_unpreserve,
>  	.freeze = vfio_pci_liveupdate_freeze,

I realize this problem wasn't introduced by this patch, but existed in the
immediately preceding patch in the series.

Is there a TOCTOU race in vfio_pci_liveupdate_freeze() that allows a device
reset while interrupts are active?

vfio_pci_liveupdate_freeze() reads vdev->irq_type to verify that interrupts
are disabled:

    if (vdev->irq_type != VFIO_PCI_NUM_IRQS) {
        pci_err(pdev, "Freeze failed! Interrupts are still enabled.\n");
        return -EINVAL;
    }

It does so without holding vdev->igate, which is the lock that protects VFIO
interrupt configuration.

Could a concurrent VFIO_DEVICE_SET_IRQS ioctl acquire vdev->igate, enable
interrupts, and update vdev->irq_type immediately after the safety check in
freeze() passes? 

freeze() would then acquire vdev->memory_lock and proceed to reset the PCI
device while interrupts are unsafely active, potentially leading to interrupt
storms or state corruption.

>  	.retrieve = vfio_pci_liveupdate_retrieve,
> +	.can_finish = vfio_pci_liveupdate_can_finish,
>  	.finish = vfio_pci_liveupdate_finish,
>  	.owner = THIS_MODULE,
>  };

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260511234802.2280368-1-vipinsh@google.com?part=3