netdev - Re: [PATCH V8 mlx5-next 08/15] vfio: Have the core code decode the VFIO_DEVICE

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: <87o82y7sp2.fsf@redhat.com>
Date:   Tue, 22 Feb 2022 17:48:57 +0100
From:   Cornelia Huck <cohuck@...hat.com>
To:     Yishai Hadas <yishaih@...dia.com>, alex.williamson@...hat.com,
        bhelgaas@...gle.com, jgg@...dia.com, saeedm@...dia.com
Cc:     linux-pci@...r.kernel.org, kvm@...r.kernel.org,
        netdev@...r.kernel.org, kuba@...nel.org, leonro@...dia.com,
        kwankhede@...dia.com, mgurtovoy@...dia.com, yishaih@...dia.com,
        maorg@...dia.com, ashok.raj@...el.com, kevin.tian@...el.com,
        shameerali.kolothum.thodi@...wei.com
Subject: Re: [PATCH V8 mlx5-next 08/15] vfio: Have the core code decode the
 VFIO_DEVICE_FEATURE ioctl

On Sun, Feb 20 2022, Yishai Hadas <yishaih@...dia.com> wrote:

> From: Jason Gunthorpe <jgg@...dia.com>
>
> Invoke a new device op 'device_feature' to handle just the data array
> portion of the command. This lifts the ioctl validation to the core code
> and makes it simpler for either the core code, or layered drivers, to
> implement their own feature values.
>
> Provide vfio_check_feature() to consolidate checking the flags/etc against
> what the driver supports.
>
> Signed-off-by: Jason Gunthorpe <jgg@...dia.com>
> Tested-by: Shameer Kolothum <shameerali.kolothum.thodi@...wei.com>
> Signed-off-by: Yishai Hadas <yishaih@...dia.com>
> ---
>  drivers/vfio/pci/vfio_pci.c      |  1 +
>  drivers/vfio/pci/vfio_pci_core.c | 94 +++++++++++++-------------------
>  drivers/vfio/vfio.c              | 46 ++++++++++++++--
>  include/linux/vfio.h             | 32 +++++++++++
>  include/linux/vfio_pci_core.h    |  2 +
>  5 files changed, 114 insertions(+), 61 deletions(-)
>

(...)

> +static int vfio_ioctl_device_feature(struct vfio_device *device,
> +				     struct vfio_device_feature __user *arg)
> +{
> +	size_t minsz = offsetofend(struct vfio_device_feature, flags);
> +	struct vfio_device_feature feature;
> +
> +	if (copy_from_user(&feature, arg, minsz))
> +		return -EFAULT;
> +
> +	if (feature.argsz < minsz)
> +		return -EINVAL;
> +
> +	/* Check unknown flags */
> +	if (feature.flags &
> +	    ~(VFIO_DEVICE_FEATURE_MASK | VFIO_DEVICE_FEATURE_SET |
> +	      VFIO_DEVICE_FEATURE_GET | VFIO_DEVICE_FEATURE_PROBE))
> +		return -EINVAL;
> +
> +	/* GET & SET are mutually exclusive except with PROBE */
> +	if (!(feature.flags & VFIO_DEVICE_FEATURE_PROBE) &&
> +	    (feature.flags & VFIO_DEVICE_FEATURE_SET) &&
> +	    (feature.flags & VFIO_DEVICE_FEATURE_GET))
> +		return -EINVAL;
> +
> +	switch (feature.flags & VFIO_DEVICE_FEATURE_MASK) {
> +	default:
> +		if (unlikely(!device->ops->device_feature))
> +			return -EINVAL;
> +		return device->ops->device_feature(device, feature.flags,
> +						   arg->data,
> +						   feature.argsz - minsz);
> +	}
> +}
> +
>  static long vfio_device_fops_unl_ioctl(struct file *filep,
>  				       unsigned int cmd, unsigned long arg)
>  {
>  	struct vfio_device *device = filep->private_data;
>  
> -	if (unlikely(!device->ops->ioctl))
> -		return -EINVAL;
> -
> -	return device->ops->ioctl(device, cmd, arg);
> +	switch (cmd) {
> +	case VFIO_DEVICE_FEATURE:
> +		return vfio_ioctl_device_feature(device, (void __user *)arg);
> +	default:
> +		if (unlikely(!device->ops->ioctl))
> +			return -EINVAL;
> +		return device->ops->ioctl(device, cmd, arg);
> +	}
>  }

One not-that-obvious change this is making is how VFIO_DEVICE_* ioctls
are processed. With this patch, VFIO_DEVICE_FEATURE is handled a bit
differently to other ioctl commands that are passed directly to the
device; here we have the common handling first, then control is passed
to the device. When I read in Documentation/driver-api/vfio.rst

"The ioctl interface provides a direct pass through for VFIO_DEVICE_*
ioctls."

I would not really expect that behaviour. No objection to introducing
it, but I think that needs a note in the doc, as you only see that if
you actually read the implementation (and not just the header and the
docs).