Re: [PATCH RFC v3 65/88] vfio/pci: Revoke guest_memfd memory on BAR invalidation and reset

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential
issues to consider:
- [High] vfio/pci: BARs marked available while pm_runtime_engaged is
  true
- [High] vfio/pci: UAF race on gmem_inode between vfio_pci_zap_bars()
  and release
- [Medium] vfio/pci: vfio_pci_core_runtime_resume() fails to restore
  bars_available
--

commit d6c318d12802531d9e53ee3087896bdf7d1f3349
Author: Ackerley Tng <ackerleytng@google.com>

vfio/pci: Revoke guest_memfd memory on BAR invalidation and reset

This commit revokes guest_memfd mappings in vfio_pci_zap_bars() when BAR
access is invalidated and marks BARs available again after memory decoding
and D0 power state are restored.

> diff --git a/drivers/vfio/pci/vfio_pci_config.c b/drivers/vfio/pci/vfio_pci_config.c
> index 9914f3ac69aef..fae3980fb8d9c 100644
> --- a/drivers/vfio/pci/vfio_pci_config.c
> +++ b/drivers/vfio/pci/vfio_pci_config.c
> @@ -630,8 +630,10 @@ static int vfio_basic_config_write(struct vfio_pci_core_device *vdev, int pos,
>  		*virt_cmd &= cpu_to_le16(~mask);
>  		*virt_cmd |= cpu_to_le16(new_cmd & mask);
>  
> -		if (__vfio_pci_memory_enabled(vdev))
> +		if (__vfio_pci_memory_enabled(vdev)) {
>  			vfio_pci_dma_buf_move(vdev, false);
> +			vfio_pci_set_bars_available(vdev, true);
> +		}
>  		up_write(&vdev->memory_lock);
>  	}

[Severity: High]
Can this set vdev->bars_available to true while vdev->pm_runtime_engaged is
still set?

When userspace enters low-power mode via VFIO_DEVICE_FEATURE_LOW_POWER_ENTRY,
vfio_pci_runtime_pm_entry() zaps BARs and sets vdev->pm_runtime_engaged =
true.  A later config write or reset ioctl wraps the access in
pm_runtime_resume_and_get() and pm_runtime_put(), so pdev->current_state is
temporarily PCI_D0 while vdev->pm_runtime_engaged remains true.  Because
__vfio_pci_memory_enabled() does not check vdev->pm_runtime_engaged, this
sets vdev->bars_available = true, and when pm_runtime_put() suspends the
device back into D3hot/D3cold without calling vfio_pci_zap_bars(),
vfio_pci_core_gmem_get_pfn() will map BAR MMIO pages of a powered-down
device.

The same applies to vfio_lock_and_set_power_state(), vfio_exp_config_write(),
vfio_af_config_write(), vfio_pci_ioctl_reset(), and
vfio_pci_dev_set_hot_reset(); unlike vfio_pci_core_gmem_attach() and
vfio_pci_vmf_insert_pfn(), none of them check !vdev->pm_runtime_engaged.

[ ... ]

> diff --git a/drivers/vfio/pci/vfio_pci_core.c b/drivers/vfio/pci/vfio_pci_core.c
> index 894b1332f8a0c..2b29d0ec6a0c9 100644
> --- a/drivers/vfio/pci/vfio_pci_core.c
> +++ b/drivers/vfio/pci/vfio_pci_core.c
> @@ -464,8 +464,10 @@ static void vfio_pci_runtime_pm_exit(struct vfio_pci_core_device *vdev)
>  	 */
>  	down_write(&vdev->memory_lock);
>  	__vfio_pci_runtime_pm_exit(vdev);
> -	if (__vfio_pci_memory_enabled(vdev))
> +	if (__vfio_pci_memory_enabled(vdev)) {
>  		vfio_pci_dma_buf_move(vdev, false);
> +		vfio_pci_set_bars_available(vdev, true);
> +	}
>  	up_write(&vdev->memory_lock);
>  }

[Severity: Medium]
Does waking from VFIO_DEVICE_FEATURE_LOW_POWER_ENTRY_WITH_WAKEUP leave
vdev->bars_available set to false?

When the device resumes with a wakeup eventfd configured,
vfio_pci_core_runtime_resume() exits low-power mode by calling
__vfio_pci_runtime_pm_exit() directly rather than vfio_pci_runtime_pm_exit():

drivers/vfio/pci/vfio_pci_core.c:vfio_pci_core_runtime_resume() {
	down_write(&vdev->memory_lock);
	if (vdev->pm_wake_eventfd_ctx) {
		eventfd_signal(vdev->pm_wake_eventfd_ctx);
		__vfio_pci_runtime_pm_exit(vdev);
	}
	up_write(&vdev->memory_lock);
}

Because vfio_pci_set_bars_available(vdev, true) is only in
vfio_pci_runtime_pm_exit(), subsequent guest_memfd BAR faults in
vfio_pci_core_gmem_get_pfn() will keep failing with -EFAULT after wakeup.

[ ... ]

> @@ -1727,8 +1731,31 @@ static void vfio_pci_zap_bars(struct vfio_pci_core_device *vdev)
>  	loff_t start = VFIO_PCI_INDEX_TO_OFFSET(VFIO_PCI_BAR0_REGION_INDEX);
>  	loff_t end = VFIO_PCI_INDEX_TO_OFFSET(VFIO_PCI_ROM_REGION_INDEX);
>  	loff_t len = end - start;
> +	int i;
>  
>  	unmap_mapping_range(core_vdev->inode->i_mapping, start, len, true);
> +
> +#ifdef CONFIG_KVM_GUEST_MEMFD
> +	if (vfio_pci_get_bars_available(vdev)) {
> +		vfio_pci_set_bars_available(vdev, false);
> +
> +		for (i = 0; i < PCI_STD_NUM_BARS; i++) {
> +			pgoff_t gmem_start;
> +			unsigned long pfn;
> +			size_t nr_pages;
> +
> +			if (!vdev->bar_mmap_supported[i])
> +				continue;
> +
> +			gmem_start = VFIO_PCI_INDEX_TO_OFFSET(i) >> PAGE_SHIFT;
> +			nr_pages = pci_resource_len(vdev->pdev, i) >> PAGE_SHIFT;
> +			pfn = pci_resource_start(vdev->pdev, i) >> PAGE_SHIFT;
> +
> +			vfio_device_gmem_revoke(&vdev->vdev, gmem_start,
> +						nr_pages, &pfn);
> +		}
> +	}
> +#endif
>  }

[ ... ]

> @@ -2726,8 +2753,7 @@ void vfio_pci_core_gmem_release(struct vfio_device *core_vdev)
>  
>  	down_write(&vdev->memory_lock);
>  
> -	/* Pairs with smp_load_acquire() in provider operations */
> -	smp_store_release(&vdev->bars_available, false);
> +	vfio_pci_set_bars_available(vdev, false);
>  
>  	up_write(&vdev->memory_lock);
>  }

[Severity: High]
Can vfio_pci_zap_bars() race with guest_memfd release and cause a
use-after-free on device->gmem_inode?

CPU0                                  CPU1
kvm_gmem_destroy_inode(inode)
  ...
  vfio_device_gmem_release()
    mutex_lock(&dev_set->lock)
    vfio_pci_core_gmem_release()
      down_write(&vdev->memory_lock)
      vfio_pci_set_bars_available(false)
      up_write(&vdev->memory_lock)
                                      vfio_pci_ioctl_reset()
                                        down_write(&vdev->memory_lock)
                                        ...
                                        vfio_pci_set_bars_available(true)
                                        up_write(&vdev->memory_lock)
                                      vfio_pci_ioctl_reset()
                                        down_write(&vdev->memory_lock)
                                        vfio_pci_zap_bars()
                                          vfio_device_gmem_revoke()
                                            loads device->gmem_inode
    device->gmem_inode = NULL
    mutex_unlock(&dev_set->lock)
  mt_destroy(&gi->attributes)
kvm_gmem_free_inode(inode)
                                            kvm_gmem_revoke(gmem_inode, ...)
                                              <-- use-after-free

vfio_device_gmem_release() clears device->gmem_inode under dev_set->lock
after vfio_pci_core_gmem_release() has already dropped vdev->memory_lock,
and even before vfio_device_gmem_release() runs, kvm_gmem_destroy_inode()
begins tearing down the evicting inode while vdev->bars_available is still
true.  Because vfio_pci_zap_bars() and vfio_pci_set_bars_available() only
take vdev->memory_lock (and set vdev->bars_available = true unconditionally
even when no guest_memfd is attached), nothing prevents
vfio_device_gmem_revoke() from dereferencing device->gmem_inode while the
inode is being destroyed and freed.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261106-gmem-alternative-providers-v3-0-8484dde66a7e@google.com?part=65