diff mbox series

[v5,14/19] vfio: Make vfio_device_open() single open for device cdev path

Message ID 20230227111135.61728-15-yi.l.liu@intel.com (mailing list archive)
State New, archived
Headers show
Series Add vfio_device cdev for iommufd support | expand

Commit Message

Yi Liu Feb. 27, 2023, 11:11 a.m. UTC 
VFIO group has historically allowed multi-open of the device FD. This
was made secure because the "open" was executed via an ioctl to the
group FD which is itself only single open.

However, no known use of multiple device FDs today. It is kind of a
strange thing to do because new device FDs can naturally be created
via dup().

When we implement the new device uAPI (only used in cdev path) there is
no natural way to allow the device itself from being multi-opened in a
secure manner. Without the group FD we cannot prove the security context
of the opener.

Thus, when moving to the new uAPI we block the ability to multi-open
the device. Old group path still allows it.

vfio_device_open() needs to sustain both the legacy behavior i.e. multi-open
in the group path and the new behavior i.e. single-open in the cdev path.
This mixture leads to the introduction of a new is_cdev_device flag in struct
vfio_device_file.

Signed-off-by: Yi Liu <yi.l.liu@intel.com>
Reviewed-by: Kevin Tian <kevin.tian@intel.com>
---
 drivers/vfio/vfio.h      |  2 ++
 drivers/vfio/vfio_main.c | 10 +++++++++-
 2 files changed, 11 insertions(+), 1 deletion(-)

Comments

Jason Gunthorpe Feb. 27, 2023, 6:52 p.m. UTC | #1 
On Mon, Feb 27, 2023 at 03:11:30AM -0800, Yi Liu wrote:
> @@ -535,7 +542,8 @@ static int vfio_device_fops_release(struct inode *inode, struct file *filep)
>  	struct vfio_device_file *df = filep->private_data;
>  	struct vfio_device *device = df->device;
>  
> -	vfio_device_group_close(df);
> +	if (!df->is_cdev_device)
> +		vfio_device_group_close(df);

This hunk should go in another patch

Jason
Yi Liu Feb. 28, 2023, 3:11 a.m. UTC | #2 
> From: Jason Gunthorpe <jgg@nvidia.com>
> Sent: Tuesday, February 28, 2023 2:52 AM
> 
> On Mon, Feb 27, 2023 at 03:11:30AM -0800, Yi Liu wrote:
> > @@ -535,7 +542,8 @@ static int vfio_device_fops_release(struct inode
> *inode, struct file *filep)
> >  	struct vfio_device_file *df = filep->private_data;
> >  	struct vfio_device *device = df->device;
> >
> > -	vfio_device_group_close(df);
> > +	if (!df->is_cdev_device)
> > +		vfio_device_group_close(df);
> 
> This hunk should go in another patch

Patch 15 or 16? Which one is your preference? To me, I guess patch
15 is better since the user may open cdev fds after it. But its release
op should not call vfio_device_group_close();

Regards,
Yi Liu
Jason Gunthorpe Feb. 28, 2023, 12:33 p.m. UTC | #3 
On Tue, Feb 28, 2023 at 03:11:34AM +0000, Liu, Yi L wrote:
> > From: Jason Gunthorpe <jgg@nvidia.com>
> > Sent: Tuesday, February 28, 2023 2:52 AM
> > 
> > On Mon, Feb 27, 2023 at 03:11:30AM -0800, Yi Liu wrote:
> > > @@ -535,7 +542,8 @@ static int vfio_device_fops_release(struct inode
> > *inode, struct file *filep)
> > >  	struct vfio_device_file *df = filep->private_data;
> > >  	struct vfio_device *device = df->device;
> > >
> > > -	vfio_device_group_close(df);
> > > +	if (!df->is_cdev_device)
> > > +		vfio_device_group_close(df);
> > 
> > This hunk should go in another patch
> 
> Patch 15 or 16? Which one is your preference? To me, I guess patch
> 15 is better since the user may open cdev fds after it. But its release
> op should not call vfio_device_group_close();

It should go with the patch that allows creating the struct file
withotu calling vfio_device_group_open()

Jason
Yi Liu March 1, 2023, 1:58 p.m. UTC | #4 
> From: Jason Gunthorpe <jgg@nvidia.com>
> Sent: Tuesday, February 28, 2023 8:34 PM
> 
> On Tue, Feb 28, 2023 at 03:11:34AM +0000, Liu, Yi L wrote:
> > > From: Jason Gunthorpe <jgg@nvidia.com>
> > > Sent: Tuesday, February 28, 2023 2:52 AM
> > >
> > > On Mon, Feb 27, 2023 at 03:11:30AM -0800, Yi Liu wrote:
> > > > @@ -535,7 +542,8 @@ static int vfio_device_fops_release(struct
> inode
> > > *inode, struct file *filep)
> > > >  	struct vfio_device_file *df = filep->private_data;
> > > >  	struct vfio_device *device = df->device;
> > > >
> > > > -	vfio_device_group_close(df);
> > > > +	if (!df->is_cdev_device)
> > > > +		vfio_device_group_close(df);
> > >
> > > This hunk should go in another patch
> >
> > Patch 15 or 16? Which one is your preference? To me, I guess patch
> > 15 is better since the user may open cdev fds after it. But its release
> > op should not call vfio_device_group_close();
> 
> It should go with the patch that allows creating the struct file
> withotu calling vfio_device_group_open()

Sure. I moved it to the patch which adds cdev as this patch starts to
have df->is_cdev_device == 1.

Regards,
Yi Liu
diff mbox series

Patch

diff --git a/drivers/vfio/vfio.h b/drivers/vfio/vfio.h
index a61d4df30716..54a48c8596f7 100644
--- a/drivers/vfio/vfio.h
+++ b/drivers/vfio/vfio.h
@@ -18,6 +18,8 @@  struct vfio_container;
 
 struct vfio_device_file {
 	struct vfio_device *device;
+	bool is_cdev_device;
+
 	bool access_granted;
 	spinlock_t kvm_ref_lock; /* protect kvm field */
 	struct kvm *kvm;
diff --git a/drivers/vfio/vfio_main.c b/drivers/vfio/vfio_main.c
index a51c0a0e8a1a..5b485d3bb67e 100644
--- a/drivers/vfio/vfio_main.c
+++ b/drivers/vfio/vfio_main.c
@@ -472,6 +472,13 @@  int vfio_device_open(struct vfio_device_file *df,
 
 	lockdep_assert_held(&device->dev_set->lock);
 
+	/*
+	 * Device cdev path cannot support multiple device open since
+	 * it doesn't have a secure way for it.
+	 */
+	if (device->open_count != 0 && df->is_cdev_device)
+		return -EINVAL;
+
 	device->open_count++;
 	if (device->open_count == 1) {
 		ret = vfio_device_first_open(df, dev_id, pt_id);
@@ -535,7 +542,8 @@  static int vfio_device_fops_release(struct inode *inode, struct file *filep)
 	struct vfio_device_file *df = filep->private_data;
 	struct vfio_device *device = df->device;
 
-	vfio_device_group_close(df);
+	if (!df->is_cdev_device)
+		vfio_device_group_close(df);
 
 	vfio_device_put_registration(device);