Re: [PATCH] vfio/pci: Do vf_token checks for VFIO_DEVICE_BIND_IOMMUFD

[Jason Gunthorpe <jgg@nvidia.com>]

On Tue, Jun 24, 2025 at 02:06:04PM -0600, Alex Williamson wrote:
> > This is used to control access to a VF unless there is co-ordination with
> > the owner of the PF.
> > 
> > Since we no longer have a device name pass the token directly though
> 
> s/name pass/name, pass/ s/though/through/

Got it
> > @@ -132,6 +132,7 @@ struct vfio_device_ops {
> >  	int	(*mmap)(struct vfio_device *vdev, struct vm_area_struct *vma);
> >  	void	(*request)(struct vfio_device *vdev, unsigned int count);
> >  	int	(*match)(struct vfio_device *vdev, char *buf);
> > +	int	(*match_token_uuid)(struct vfio_device *vdev, const uuid_t *uuid);
> >  	void	(*dma_unmap)(struct vfio_device *vdev, u64 iova, u64 length);
> >  	int	(*device_feature)(struct vfio_device *device, u32 flags,
> >  				  void __user *arg, size_t argsz);
> 
> Update the structure comments.

 * @match_token_uuid: Optional device token match/validation. Return 0
 *         if the uuid is valid for the device, -errno otherwise. uuid is NULL
 *         if none was provided.

> > diff --git a/include/linux/vfio_pci_core.h b/include/linux/vfio_pci_core.h
> > index fbb472dd99b361..f541044e42a2ad 100644
> > --- a/include/linux/vfio_pci_core.h
> > +++ b/include/linux/vfio_pci_core.h
> > @@ -122,6 +122,8 @@ ssize_t vfio_pci_core_write(struct vfio_device *core_vdev, const char __user *bu
> >  int vfio_pci_core_mmap(struct vfio_device *core_vdev, struct vm_area_struct *vma);
> >  void vfio_pci_core_request(struct vfio_device *core_vdev, unsigned int count);
> >  int vfio_pci_core_match(struct vfio_device *core_vdev, char *buf);
> > +int vfio_pci_core_match_token_uuid(struct vfio_device *core_vdev,
> > +				   const uuid_t *uuid);
> >  int vfio_pci_core_enable(struct vfio_pci_core_device *vdev);
> >  void vfio_pci_core_disable(struct vfio_pci_core_device *vdev);
> >  void vfio_pci_core_finish_enable(struct vfio_pci_core_device *vdev);
> > diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h
> > index 5764f315137f99..48233ec4daf7b4 100644
> > --- a/include/uapi/linux/vfio.h
> > +++ b/include/uapi/linux/vfio.h
> > @@ -901,14 +901,18 @@ struct vfio_device_feature {
> >  
> >  #define VFIO_DEVICE_FEATURE		_IO(VFIO_TYPE, VFIO_BASE + 17)
> >  
> > +#define VFIO_DEVICE_BIND_TOKEN (1 << 0)
> 
> We tend to define ioctl flags within the ioctl data structure and
> include "_FLAG_" in the name.

 */
struct vfio_device_bind_iommufd {
	__u32		argsz;
	__u32		flags;
#define VFIO_DEVICE_BIND_FLAG_TOKEN (1 << 0)
	__s32		iommufd;

> > @@ -924,6 +934,7 @@ struct vfio_device_bind_iommufd {
> >  	__u32		flags;
> >  	__s32		iommufd;
> >  	__u32		out_devid;
> > +	__aligned_u64	token_uuid_ptr;
> >  };
> 
> So we're expecting in the general case, old code doesn't set the flag,
> doesn't need a token, continues to work.

Yes

> There's potentially a narrow case of old code that should have
> required a token, which now intentionally breaks.

Yes

> We're not offering an introspection mechanism
> here, but doing so also doesn't add a lot of value. 

Right.

> Userspace needs to know the token to pass anyway.  Is that how you
> see it?

Yes, we are fixing a security bug here.
 
> Do note that QEMU already has support for this in the legacy interface
> and should just need to reparse the token from the name provided
> through the attach_device callback and pass it through to the
> iommufd_cdev_connect_and_bind() function.

Yes, that sounds right.

I will repost it and hopefully someone has an easy test environment

Jason