linux-kernel - RE: [PATCH v2 13/14] vfio: Add ioctls for device cdev using iommufd

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <BN9PR11MB5276A5DEB3855788C02A343B8CDB9@BN9PR11MB5276.namprd11.prod.outlook.com>
Date:   Tue, 7 Feb 2023 09:17:29 +0000
From:   "Tian, Kevin" <kevin.tian@...el.com>
To:     "Liu, Yi L" <yi.l.liu@...el.com>,
        "alex.williamson@...hat.com" <alex.williamson@...hat.com>,
        "jgg@...dia.com" <jgg@...dia.com>
CC:     "cohuck@...hat.com" <cohuck@...hat.com>,
        "eric.auger@...hat.com" <eric.auger@...hat.com>,
        "nicolinc@...dia.com" <nicolinc@...dia.com>,
        "kvm@...r.kernel.org" <kvm@...r.kernel.org>,
        "mjrosato@...ux.ibm.com" <mjrosato@...ux.ibm.com>,
        "chao.p.peng@...ux.intel.com" <chao.p.peng@...ux.intel.com>,
        "yi.y.sun@...ux.intel.com" <yi.y.sun@...ux.intel.com>,
        "peterx@...hat.com" <peterx@...hat.com>,
        "jasowang@...hat.com" <jasowang@...hat.com>,
        "shameerali.kolothum.thodi@...wei.com" 
        <shameerali.kolothum.thodi@...wei.com>,
        "lulu@...hat.com" <lulu@...hat.com>,
        "suravee.suthikulpanit@....com" <suravee.suthikulpanit@....com>,
        "intel-gvt-dev@...ts.freedesktop.org" 
        <intel-gvt-dev@...ts.freedesktop.org>,
        "intel-gfx@...ts.freedesktop.org" <intel-gfx@...ts.freedesktop.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux-s390@...r.kernel.org" <linux-s390@...r.kernel.org>
Subject: RE: [PATCH v2 13/14] vfio: Add ioctls for device cdev using iommufd

> From: Liu, Yi L <yi.l.liu@...el.com>
> Sent: Monday, February 6, 2023 5:06 PM
> 
> +long vfio_device_ioctl_bind_iommufd(struct vfio_device_file *df,
> +				    unsigned long arg)
> +{
> +	struct vfio_device *device = df->device;
> +	struct vfio_device_bind_iommufd bind;
> +	struct iommufd_ctx *iommufd = NULL;
> +	unsigned long minsz;
> +	struct fd f;
> +	int ret;
> +
> +	minsz = offsetofend(struct vfio_device_bind_iommufd, iommufd);

shouldn't the minsz include out_devid?

> +	/*
> +	 * Before the first device open, get the KVM pointer currently
> +	 * associated with the device file (if there is) and obtain a
> +	 * reference. This reference is held until device closed. Save
> +	 * the pointer in the device for use by drivers.
> +	 */
> +	vfio_device_get_kvm_safe(df);

there is no multi-open for cdev so "before the first device open"
doesn't make sense here.

> +int vfio_ioctl_device_attach(struct vfio_device_file *df,
> +			     void __user *arg)
> +{
> +	struct vfio_device *device = df->device;
> +	struct vfio_device_attach_iommufd_pt attach;
> +	int ret;
> +
> +	if (copy_from_user(&attach, (void __user *)arg, sizeof(attach)))
> +		return -EFAULT;

lack of a minsz check

> +
> +	if (attach.flags || attach.pt_id == IOMMUFD_INVALID_ID)
> +		return -EINVAL;
> +
> +	if (!device->ops->bind_iommufd)
> +		return -ENODEV;
> +
> +	mutex_lock(&device->dev_set->lock);
> +	if (df->noiommu)
> +		return -ENODEV;

-EINVAL

> +
> +int vfio_ioctl_device_detach(struct vfio_device_file *df,
> +			     void __user *arg)
> +{
> +	struct vfio_device *device = df->device;
> +	struct vfio_device_detach_iommufd_pt detach;
> +
> +	if (copy_from_user(&detach, (void __user *)arg, sizeof(detach)))
> +		return -EFAULT;

lack of minsz check

> +	mutex_lock(&device->dev_set->lock);
> +	if (df->noiommu)
> +		return -ENODEV;

-EINVAL

> @@ -442,16 +443,41 @@ static int vfio_device_first_open(struct
> vfio_device_file *df,
>  {
>  	struct vfio_device *device = df->device;
>  	struct iommufd_ctx *iommufd = df->iommufd;
> -	int ret;
> +	int ret = 0;
> 
>  	lockdep_assert_held(&device->dev_set->lock);
> 
> +	/* df->iommufd and df->noiommu should be exclusive */
> +	if (WARN_ON(iommufd && df->noiommu))

pointless comment

> +		return -EINVAL;
> +
>  	if (!try_module_get(device->dev->driver->owner))
>  		return -ENODEV;
> 
> +	/*
> +	 * For group path, iommufd pointer is NULL when comes into this

"For group/container path"

> +	 * helper. Its noiommu support is in container.c.
> +	 *
> +	 * For iommufd compat mode, iommufd pointer here is a valid value.
> +	 * Its noiommu support is supposed to be in vfio_iommufd_bind().

remove "supposed to be"

> +	 *
> +	 * For device cdev path, iommufd pointer here is a valid value for
> +	 * normal cases, but it is NULL if it's noiommu. The reason is
> +	 * that userspace uses iommufd==-1 to indicate noiommu mode in
> this

"iommufd < 0"

> +	 * path. So caller of this helper will pass in a NULL iommufd

I don't think that is the reason. Instead the caller is required to pass
NULL iommufd pointer to indicate noiommu. Just remove the reason part.

> +	 * pointer. To differentiate it from the group path which also
> +	 * passes NULL iommufd pointer in, df->noiommu is used. For cdev
> +	 * noiommu, df->noiommu would be set to mark noiommu case for
> cdev
> +	 * path.

"To differentiate ..., check df->noiommu which is set only in the cdev path"

> +	 *
> +	 * So if df->noiommu is set then this helper just goes ahead to
> +	 * open device. If not, it depends on if iommufd pointer is NULL
> +	 * to handle the group path, iommufd compat mode, normal cases in
> +	 * the cdev path.

this doesn't match the code order. Probably can be removed after earlier
explanation.

> + * @argsz:	 user filled size of this data.
> + * @flags:	 reserved for future extension.
> + * @dev_cookie:	 a per device cookie provided by userspace.
> + * @iommufd:	 iommufd to bind. iommufd < 0 means noiommu.

s/iommufd < 0/a negative value/