[v2,11/14] vfio: Make vfio_device_open() exclusive between group path and device cdev path
Commit Message
With the introduction of vfio device cdev, userspace can get device
access by either the legacy group path or the cdev path. For VFIO devices,
it can only be opened by one of the group path and the cdev path at one
time. e.g. when the device is opened via cdev path, the group path should
be failed. Both paths will call into vfio_device_open(), so the exclusion
is done in it.
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. This requires vfio_device_open()
exclusive between the cdev path with the group path.
The main logic is in the vfio_device_open(). It 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,
and a single_open flag in struct vfio_device.
- vfio_device_file::is_cdev_device is set per the vfio_device_file
allocation.
- vfio_device::single_open is set after open_device op is called
successfully if vfio_device_file::is_cdev_device is set.
Signed-off-by: Yi Liu <yi.l.liu@intel.com>
---
drivers/vfio/group.c | 2 +-
drivers/vfio/vfio.h | 4 +++-
drivers/vfio/vfio_main.c | 26 +++++++++++++++++++++++---
include/linux/vfio.h | 1 +
4 files changed, 28 insertions(+), 5 deletions(-)
Comments
> From: Liu, Yi L <yi.l.liu@intel.com>
> Sent: Monday, February 6, 2023 5:05 PM
>
> struct vfio_device_file *
> -vfio_allocate_device_file(struct vfio_device *device)
> +vfio_allocate_device_file(struct vfio_device *device, bool is_cdev_device)
> {
> struct vfio_device_file *df;
>
> @@ -407,6 +407,7 @@ vfio_allocate_device_file(struct vfio_device *device)
> return ERR_PTR(-ENOMEM);
>
> df->device = device;
> + df->is_cdev_device = is_cdev_device;
You missed Alex's comment that the one caller can open code the
assignment instead of introducing an unmemorable Boolean arg here.
>
> + /*
> + * Device cdev path cannot support multiple device open since
> + * it doesn't have a secure way for it. So a second device
> + * open attempt should be failed if the caller is from a cdev
> + * path or the device has already been opened by a cdev path.
> + */
> + if (device->open_count != 0 &&
> + (df->is_cdev_device || device->single_open))
> + return -EINVAL;
> +
If we are gonna handle the exclusive open via dma ownership, then
here we don't need a new single_open flag inside vfio_device since
only one interface (cdev or group) could be used to open this device.
Just preventing multi-open in cdev is sufficient here.
> From: Tian, Kevin <kevin.tian@intel.com>
> Sent: Tuesday, February 7, 2023 2:25 PM
>
> > From: Liu, Yi L <yi.l.liu@intel.com>
> > Sent: Monday, February 6, 2023 5:05 PM
> >
> > struct vfio_device_file *
> > -vfio_allocate_device_file(struct vfio_device *device)
> > +vfio_allocate_device_file(struct vfio_device *device, bool
> is_cdev_device)
> > {
> > struct vfio_device_file *df;
> >
> > @@ -407,6 +407,7 @@ vfio_allocate_device_file(struct vfio_device
> *device)
> > return ERR_PTR(-ENOMEM);
> >
> > df->device = device;
> > + df->is_cdev_device = is_cdev_device;
>
> You missed Alex's comment that the one caller can open code the
> assignment instead of introducing an unmemorable Boolean arg here.
Oh, yes. will open code to set it in cdev path.
> >
> > + /*
> > + * Device cdev path cannot support multiple device open since
> > + * it doesn't have a secure way for it. So a second device
> > + * open attempt should be failed if the caller is from a cdev
> > + * path or the device has already been opened by a cdev path.
> > + */
> > + if (device->open_count != 0 &&
> > + (df->is_cdev_device || device->single_open))
> > + return -EINVAL;
> > +
>
> If we are gonna handle the exclusive open via dma ownership, then
> here we don't need a new single_open flag inside vfio_device since
> only one interface (cdev or group) could be used to open this device.
>
> Just preventing multi-open in cdev is sufficient here.
I see. Per below discussion, just need to make group path always
use vfio_group pointer as DMA marker.
https://lore.kernel.org/kvm/BN9PR11MB5276FA68E8CAD6394A8887848CDB9@BN9PR11MB5276.namprd11.prod.outlook.com/
Regards,
Yi Liu
@@ -237,7 +237,7 @@ static struct file *vfio_device_open_file(struct vfio_device *device)
struct file *filep;
int ret;
- df = vfio_allocate_device_file(device);
+ df = vfio_allocate_device_file(device, false);
if (IS_ERR(df)) {
ret = PTR_ERR(df);
goto err_out;
@@ -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;
@@ -30,7 +32,7 @@ int vfio_device_open(struct vfio_device_file *df,
u32 *dev_id, u32 *pt_id);
void vfio_device_close(struct vfio_device_file *df);
struct vfio_device_file *
-vfio_allocate_device_file(struct vfio_device *device);
+vfio_allocate_device_file(struct vfio_device *device, bool is_cdev_device);
extern const struct file_operations vfio_device_fops;
@@ -398,7 +398,7 @@ static bool vfio_assert_device_open(struct vfio_device *device)
}
struct vfio_device_file *
-vfio_allocate_device_file(struct vfio_device *device)
+vfio_allocate_device_file(struct vfio_device *device, bool is_cdev_device)
{
struct vfio_device_file *df;
@@ -407,6 +407,7 @@ vfio_allocate_device_file(struct vfio_device *device)
return ERR_PTR(-ENOMEM);
df->device = device;
+ df->is_cdev_device = is_cdev_device;
spin_lock_init(&df->kvm_ref_lock);
return df;
@@ -472,11 +473,23 @@ 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. So a second device
+ * open attempt should be failed if the caller is from a cdev
+ * path or the device has already been opened by a cdev path.
+ */
+ if (device->open_count != 0 &&
+ (df->is_cdev_device || device->single_open))
+ return -EINVAL;
+
device->open_count++;
if (device->open_count == 1) {
ret = vfio_device_first_open(df, dev_id, pt_id);
if (ret)
device->open_count--;
+ else
+ device->single_open = df->is_cdev_device;
}
if (ret)
@@ -497,8 +510,10 @@ void vfio_device_close(struct vfio_device_file *df)
lockdep_assert_held(&device->dev_set->lock);
vfio_assert_device_open(device);
- if (device->open_count == 1)
+ if (device->open_count == 1) {
vfio_device_last_close(df);
+ device->single_open = false; // clear single_open flag
+ }
device->open_count--;
}
@@ -543,7 +558,12 @@ 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);
+ /*
+ * group path supports multiple device open, while cdev doesn't.
+ * So use vfio_device_group_close() for !singel_open case.
+ */
+ if (!df->is_cdev_device)
+ vfio_device_group_close(df);
vfio_device_put_registration(device);
@@ -63,6 +63,7 @@ struct vfio_device {
struct iommufd_ctx *iommufd_ictx;
bool iommufd_attached;
#endif
+ bool single_open;
};
/**