linux-kernel - Re: [PATCH v9 5/7] vfio/type1: also check IRQ remapping capability at msi domain

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: <20160509164942.5af8b30d@t450s.home>
Date:	Mon, 9 May 2016 16:49:42 -0600
From:	Alex Williamson <alex.williamson@...hat.com>
To:	Eric Auger <eric.auger@...aro.org>
Cc:	eric.auger@...com, robin.murphy@....com, will.deacon@....com,
	joro@...tes.org, tglx@...utronix.de, jason@...edaemon.net,
	marc.zyngier@....com, christoffer.dall@...aro.org,
	linux-arm-kernel@...ts.infradead.org, patches@...aro.org,
	linux-kernel@...r.kernel.org, Bharat.Bhushan@...escale.com,
	pranav.sawargaonkar@...il.com, p.fedin@...sung.com,
	iommu@...ts.linux-foundation.org, Jean-Philippe.Brucker@....com,
	julien.grall@....com, yehuday@...vell.com
Subject: Re: [PATCH v9 5/7] vfio/type1: also check IRQ remapping capability
 at msi domain

On Wed,  4 May 2016 11:54:16 +0000
Eric Auger <eric.auger@...aro.org> wrote:

> On x86 IRQ remapping is abstracted by the IOMMU. On ARM this is abstracted
> by the msi controller. vfio_safe_irq_domain allows to check whether
> interrupts are "safe" for a given device. They are if the device does
> not use MSI

Are we sure we're not opening a security hole here?  An MSI is simply a
DMA write, so really whether or not a device uses MSI is irrelevant.
If it can generate a DMA to the MSI doorbell then we need to be
protected and I think we pretty much need to assume that devices are
DMA capable.  Do the MSI domain checks cover this?

> or if the device uses MSI and the msi-parent controller
> supports IRQ remapping.
> 
> Then we check at group level if all devices have safe interrupts: if not,
> we only allow the group to be attached if allow_unsafe_interrupts is set.
> 
> At this point ARM sMMU still advertises IOMMU_CAP_INTR_REMAP. This is
> changed in next patch.
> 
> Signed-off-by: Eric Auger <eric.auger@...aro.org>
> 
> ---
> v3 -> v4:
> - rename vfio_msi_parent_irq_remapping_capable into vfio_safe_irq_domain
>   and irq_remapping into safe_irq_domains
> 
> v2 -> v3:
> - protect vfio_msi_parent_irq_remapping_capable with
>   CONFIG_GENERIC_MSI_IRQ_DOMAIN
> ---
>  drivers/vfio/vfio_iommu_type1.c | 44 +++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 42 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c
> index 4d3a6f1..2fc8197 100644
> --- a/drivers/vfio/vfio_iommu_type1.c
> +++ b/drivers/vfio/vfio_iommu_type1.c
> @@ -37,6 +37,8 @@
>  #include <linux/vfio.h>
>  #include <linux/workqueue.h>
>  #include <linux/msi-iommu.h>
> +#include <linux/irqdomain.h>
> +#include <linux/msi.h>
>  
>  #define DRIVER_VERSION  "0.2"
>  #define DRIVER_AUTHOR   "Alex Williamson <alex.williamson@...hat.com>"
> @@ -777,6 +779,33 @@ static int vfio_bus_type(struct device *dev, void *data)
>  	return 0;
>  }
>  
> +/**
> + * vfio_safe_irq_domain: returns whether the irq domain
> + * the device is attached to is safe with respect to MSI isolation.
> + * If the irq domain is not an MSI domain, we return it is safe.
> + *
> + * @dev: device handle
> + * @data: unused
> + * returns 0 if the irq domain is safe, -1 if not.
> + */
> +static int vfio_safe_irq_domain(struct device *dev, void *data)
> +{
> +#ifdef CONFIG_GENERIC_MSI_IRQ_DOMAIN
> +	struct irq_domain *domain;
> +	struct msi_domain_info *info;
> +
> +	domain = dev_get_msi_domain(dev);
> +	if (!domain)
> +		return 0;
> +
> +	info = msi_get_domain_info(domain);
> +
> +	if (!(info->flags & MSI_FLAG_IRQ_REMAPPING))
> +		return -1;
> +#endif
> +	return 0;
> +}
> +
>  static int vfio_iommu_replay(struct vfio_iommu *iommu,
>  			     struct vfio_domain *domain)
>  {
> @@ -870,7 +899,7 @@ static int vfio_iommu_type1_attach_group(void *iommu_data,
>  	struct vfio_group *group, *g;
>  	struct vfio_domain *domain, *d;
>  	struct bus_type *bus = NULL;
> -	int ret;
> +	int ret, safe_irq_domains;
>  
>  	mutex_lock(&iommu->lock);
>  
> @@ -893,6 +922,13 @@ static int vfio_iommu_type1_attach_group(void *iommu_data,
>  
>  	group->iommu_group = iommu_group;
>  
> +	/*
> +	 * Determine if all the devices of the group have a safe irq domain
> +	 * with respect to MSI isolation
> +	 */
> +	safe_irq_domains = !iommu_group_for_each_dev(iommu_group, &bus,
> +				       vfio_safe_irq_domain);
> +
>  	/* Determine bus_type in order to allocate a domain */
>  	ret = iommu_group_for_each_dev(iommu_group, &bus, vfio_bus_type);
>  	if (ret)
> @@ -920,8 +956,12 @@ static int vfio_iommu_type1_attach_group(void *iommu_data,
>  	INIT_LIST_HEAD(&domain->group_list);
>  	list_add(&group->next, &domain->group_list);
>  
> +	/*
> +	 * to advertise safe interrupts either the IOMMU or the MSI controllers
> +	 * must support IRQ remapping/interrupt translation
> +	 */
>  	if (!allow_unsafe_interrupts &&
> -	    !iommu_capable(bus, IOMMU_CAP_INTR_REMAP)) {
> +	    (!iommu_capable(bus, IOMMU_CAP_INTR_REMAP) && !safe_irq_domains)) {
>  		pr_warn("%s: No interrupt remapping support.  Use the module param \"allow_unsafe_interrupts\" to enable VFIO IOMMU support on this platform\n",
>  		       __func__);
>  		ret = -EPERM;