Re: [PATCH 7/7] vfio: Use driver_override to avert binding to compromising drivers

[Auger Eric]

Hi Alex,

On 10/06/2017 00:00, Alex Williamson wrote:
> If a device is bound to a non-vfio, non-whitelisted driver while a
> group is in use, then the integrity of the group is compromised and
> will result in hitting a BUG_ON.  This code tries to avoid this case
> by mangling driver_override to force a no-match for the driver.  The
> driver-core will either follow-up with a DRIVER_NOT_BOUND (preferred)
> or BOUND_DRIVER, at which point we can remove the driver_override
> mangling.
> 
> A complication here is that even though the window between these
> notifications is expected to be extremely small, the vfio group could
> be removed, which would prevent us from finding the group again to
> remove the driver_override.  We therefore take a group reference when
> adding to driver_override and release it when removed.  A second
> complication is that driver_override can be modified by the system
> admin through sysfs.  To avoid trivial interference, we add a non-
> user-visible UUID to the group and use this as part of the mangle
> string.
> 
> The above blocks binding to a driver that would compromise the host,
> but we'd also like to avoid reaching that step if possible.  For this
> we add a wait_event_timeout() with a short, 1 second timeout, which is
> highly effective in allowing empty groups to finish cleanup.
> 
> Signed-off-by: Alex Williamson <alex.williamson@xxxxxxxxxx>
> ---
>  drivers/vfio/vfio.c |  151 +++++++++++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 144 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/vfio/vfio.c b/drivers/vfio/vfio.c
> index a25ee4930200..ea786d512faa 100644
> --- a/drivers/vfio/vfio.c
> +++ b/drivers/vfio/vfio.c
> @@ -25,6 +25,7 @@
>  #include <linux/miscdevice.h>
>  #include <linux/module.h>
>  #include <linux/mutex.h>
> +#include <linux/platform_device.h>
>  #include <linux/pci.h>
>  #include <linux/rwsem.h>
>  #include <linux/sched.h>
> @@ -32,8 +33,10 @@
>  #include <linux/stat.h>
>  #include <linux/string.h>
>  #include <linux/uaccess.h>
> +#include <linux/uuid.h>
>  #include <linux/vfio.h>
>  #include <linux/wait.h>
> +#include <linux/amba/bus.h>
>  
>  #define DRIVER_VERSION	"0.3"
>  #define DRIVER_AUTHOR	"Alex Williamson <alex.williamson@xxxxxxxxxx>"
> @@ -95,6 +98,7 @@ struct vfio_group {
>  	bool				noiommu;
>  	struct kvm			*kvm;
>  	struct blocking_notifier_head	notifier;
> +	unsigned char			uuid[16];
>  };
>  
>  struct vfio_device {
> @@ -352,6 +356,8 @@ static struct vfio_group *vfio_create_group(struct iommu_group *iommu_group)
>  
>  	group->nb.notifier_call = vfio_iommu_group_notifier;
>  
> +	generate_random_uuid(group->uuid);
> +
>  	/*
>  	 * blocking notifiers acquire a rwsem around registering and hold
>  	 * it around callback.  Therefore, need to register outside of
> @@ -728,6 +734,116 @@ static int vfio_group_nb_verify(struct vfio_group *group, struct device *dev)
>  	return vfio_dev_viable(dev, group);
>  }
>  
> +#define VFIO_TAG_PREFIX "#vfio_group:"
> +
> +static char **vfio_find_driver_override(struct device *dev)
> +{
> +	if (dev_is_pci(dev)) {
> +		struct pci_dev *pdev = to_pci_dev(dev);
> +		return &pdev->driver_override;
> +	} else if (dev->bus == &platform_bus_type) {
> +		struct platform_device *pdev = to_platform_device(dev);
> +		return &pdev->driver_override;
> +#ifdef CONFIG_ARM_AMBA
> +	} else if (dev->bus == &amba_bustype) {

EXPORT_SYMBOL_GPL(amba_bustype);
needs to be added in drivers/amba/bus.c otherwise this causes a link error
ERROR: "amba_bustype" [drivers/vfio/vfio.ko] undefined!

Otherwise it looks good to me and I 've tested this with dual port igb
on ARM.

Reviewed-by: Eric Auger <eric.auger@xxxxxxxxxx>
Tested-by: Eric Auger <eric.auger@xxxxxxxxxx>

Thanks

Eric


> +		struct amba_device *adev = to_amba_device(dev);
> +		return &adev->driver_override;
> +#endif
> +	}
> +
> +	return NULL;
> +}
> +
> +/*
> + * If we're about to bind to something other than a known whitelisted driver
> + * or known vfio bus driver, try to avert it with driver_override.
> + */
> +static void vfio_group_nb_pre_bind(struct vfio_group *group, struct device *dev)
> +{
> +	struct vfio_bus_driver *driver;
> +	struct device_driver *drv = ACCESS_ONCE(dev->driver);
> +	char **driver_override;
> +
> +	if (vfio_dev_whitelisted(dev, drv))
> +		return; /* Binding to known "innocuous" device/driver */
> +
> +	mutex_lock(&vfio.bus_drivers_lock);
> +	list_for_each_entry(driver, &vfio.bus_drivers_list, vfio_next) {
> +		if (driver->drv == drv) {
> +			mutex_unlock(&vfio.bus_drivers_lock);
> +			return; /* Binding to known vfio bus driver, ok */
> +		}
> +	}
> +	mutex_unlock(&vfio.bus_drivers_lock);
> +
> +	/* Can we stall slightly to let users fall off? */
> +	if (list_empty(&group->device_list)) {
> +		if (wait_event_timeout(vfio.release_q,
> +				!atomic_read(&group->container_users), HZ))
> +			return;
> +	}
> +
> +	driver_override = vfio_find_driver_override(dev);
> +	if (driver_override) {
> +		char tag[50], *new = NULL, *old = *driver_override;
> +
> +		snprintf(tag, sizeof(tag), "%s%pU",
> +			 VFIO_TAG_PREFIX, group->uuid);
> +
> +		if (old && strstr(old, tag))
> +			return; /* block already in place */
> +
> +		new = kasprintf(GFP_KERNEL, "%s%s", old ? old : "", tag);
> +		if (new) {
> +			*driver_override = new;
> +			kfree(old);
> +			vfio_group_get(group);
> +			dev_warn(dev, "vfio: Blocking unsafe driver bind\n");
> +			return;
> +		}
> +	}
> +
> +	dev_warn(dev, "vfio: Unsafe driver binding to in-use group!\n");
> +}
> +
> +/* If we've mangled driver_override, remove it */
> +static void vfio_group_nb_post_bind(struct vfio_group *group,
> +				    struct device *dev)
> +{
> +	char **driver_override = vfio_find_driver_override(dev);
> +
> +	if (driver_override && *driver_override) {
> +		char tag[50], *new, *start, *end, *old = *driver_override;
> +
> +		snprintf(tag, sizeof(tag), "%s%pU",
> +			 VFIO_TAG_PREFIX, group->uuid);
> +
> +		start = strstr(old, tag);
> +		if (start) {
> +			end = start + strlen(tag);
> +
> +			if (old + strlen(old) > end)
> +				memmove(start, end,
> +					strlen(old) - (end - old) + 1);
> +			else
> +				*start = 0;
> +
> +			if (strlen(old)) {
> +				new = kasprintf(GFP_KERNEL, "%s", old);
> +				if (new) {
> +					*driver_override = new;
> +					kfree(old);
> +				} /* else, in-place terminated, ok */
> +			} else {
> +				*driver_override = NULL;
> +				kfree(old);
> +			}
> +
> +			vfio_group_put(group);
> +		}
> +	}
> +}
> +
>  static int vfio_iommu_group_notifier(struct notifier_block *nb,
>  				     unsigned long action, void *data)
>  {
> @@ -757,14 +873,23 @@ static int vfio_iommu_group_notifier(struct notifier_block *nb,
>  		 */
>  		break;
>  	case IOMMU_GROUP_NOTIFY_BIND_DRIVER:
> -		pr_debug("%s: Device %s, group %d binding to driver\n",
> +		pr_debug("%s: Device %s, group %d binding to driver %s\n",
>  			 __func__, dev_name(dev),
> -			 iommu_group_id(group->iommu_group));
> +			 iommu_group_id(group->iommu_group), dev->driver->name);
> +		if (vfio_group_nb_verify(group, dev))
> +			vfio_group_nb_pre_bind(group, dev);
> +		break;
> +	case IOMMU_GROUP_NOTIFY_DRIVER_NOT_BOUND:
> +		pr_debug("%s: Device %s, group %d binding fail to driver %s\n",
> +			 __func__, dev_name(dev),
> +			 iommu_group_id(group->iommu_group), dev->driver->name);
> +		vfio_group_nb_post_bind(group, dev);
>  		break;
>  	case IOMMU_GROUP_NOTIFY_BOUND_DRIVER:
>  		pr_debug("%s: Device %s, group %d bound to driver %s\n",
>  			 __func__, dev_name(dev),
>  			 iommu_group_id(group->iommu_group), dev->driver->name);
> +		vfio_group_nb_post_bind(group, dev);
>  		BUG_ON(vfio_group_nb_verify(group, dev));
>  		break;
>  	case IOMMU_GROUP_NOTIFY_UNBIND_DRIVER:
> @@ -1351,6 +1476,7 @@ static int vfio_group_unset_container(struct vfio_group *group)
>  	if (users != 1)
>  		return -EBUSY;
>  
> +	wake_up(&vfio.release_q);
>  	__vfio_group_unset_container(group);
>  
>  	return 0;
> @@ -1364,7 +1490,11 @@ static int vfio_group_unset_container(struct vfio_group *group)
>   */
>  static void vfio_group_try_dissolve_container(struct vfio_group *group)
>  {
> -	if (0 == atomic_dec_if_positive(&group->container_users))
> +	int users = atomic_dec_if_positive(&group->container_users);
> +
> +	wake_up(&vfio.release_q);
> +
> +	if (!users)
>  		__vfio_group_unset_container(group);
>  }
>  
> @@ -1433,19 +1563,26 @@ static bool vfio_group_viable(struct vfio_group *group)
>  
>  static int vfio_group_add_container_user(struct vfio_group *group)
>  {
> +	int ret;
> +
>  	if (!atomic_inc_not_zero(&group->container_users))
>  		return -EINVAL;
>  
>  	if (group->noiommu) {
> -		atomic_dec(&group->container_users);
> -		return -EPERM;
> +		ret = -EPERM;
> +		goto out;
>  	}
>  	if (!group->container->iommu_driver || !vfio_group_viable(group)) {
> -		atomic_dec(&group->container_users);
> -		return -EINVAL;
> +		ret = -EINVAL;
> +		goto out;
>  	}
>  
>  	return 0;
> +
> +out:
> +	atomic_dec(&group->container_users);
> +	wake_up(&vfio.release_q);
> +	return ret;
>  }
>  
>  static const struct file_operations vfio_device_fops;
>