linux-kernel - Re: [RFC PATCH 1/1] vfio-pci/iommu: Detach iommu group on remove path

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] [day] [month] [year] [list]

Message-ID: <20150723152752.5454e4b4@thinkpad>
Date:	Thu, 23 Jul 2015 15:27:52 +0200
From:	Gerald Schaefer <gerald.schaefer@...ibm.com>
To:	Alex Williamson <alex.williamson@...hat.com>,
	Joerg Roedel <joro@...tes.org>
Cc:	kvm@...r.kernel.org, linux-kernel@...r.kernel.org,
	Martin Schwidefsky <schwidefsky@...ibm.com>,
	Sebastian Ott <sebott@...ux.vnet.ibm.com>,
	iommu@...ts.linux-foundation.org
Subject: Re: [RFC PATCH 1/1] vfio-pci/iommu: Detach iommu group on remove
 path

On Wed, 22 Jul 2015 11:10:57 -0600
Alex Williamson <alex.williamson@...hat.com> wrote:

> On Wed, 2015-07-22 at 10:54 -0600, Alex Williamson wrote:
> > On Tue, 2015-07-21 at 19:44 +0200, Gerald Schaefer wrote:
> > > When a user completes the VFIO_SET_IOMMU ioctl and the vfio-pci
> > > device is removed thereafter (before any other ioctl like
> > > VFIO_GROUP_GET_DEVICE_FD), then the detach_dev callback of the
> > > underlying IOMMU API is never called.
> > > 
> > > This patch adds a call to vfio_group_try_dissolve_container() to
> > > the remove path, which will trigger the missing detach_dev
> > > callback in this scenario.
> > > 
> > > Signed-off-by: Gerald Schaefer <gerald.schaefer@...ibm.com>
> > > ---
> > >  drivers/vfio/vfio.c | 3 +++
> > >  1 file changed, 3 insertions(+)
> > > 
> > > diff --git a/drivers/vfio/vfio.c b/drivers/vfio/vfio.c
> > > index 2fb29df..9c5c784 100644
> > > --- a/drivers/vfio/vfio.c
> > > +++ b/drivers/vfio/vfio.c
> > > @@ -711,6 +711,8 @@ static bool vfio_dev_present(struct
> > > vfio_group *group, struct device *dev) return true;
> > >  }
> > >  
> > > +static void vfio_group_try_dissolve_container(struct vfio_group
> > > *group); +
> > >  /*
> > >   * Decrement the device reference count and wait for the device
> > > to be
> > >   * removed.  Open file descriptors for the device... */
> > > @@ -785,6 +787,7 @@ void *vfio_del_group_dev(struct device *dev)
> > >  		}
> > >  	} while (ret <= 0);
> > >  
> > > +	vfio_group_try_dissolve_container(group);
> > >  	vfio_group_put(group);
> > >  
> > >  	return device_data;
> > 
> > 
> > This won't work, vfio_group_try_dissolve_container() decrements
> > container_users, which an unused device is not.  Imagine if we had
> > more than one device in the iommu group, one device is removed and
> > the container is dissolved despite the user holding a reference and
> > other viable devices remaining.  Additionally, from an isolation
> > perspective, an unbind from vfio-pci should not pull the device out
> > of the iommu domain, it's part of the domain because it's not
> > isolated and that continues even after unbind.
> > 
> > I think what you want to do is detach a device from the iommu domain
> > only when it's being removed from iommu group, such as through
> > iommu_group_remove_device().  We already have a bit of an asymmetry
> > there as iommu_group_add_device() will add devices to the currently
> > active iommu domain for the group, but iommu_group_remove_device()
> > does not appear to do the reverse.  Thanks,
> 
> BTW, VT-d on x86 avoids a leak using its own notifier_block,
> drivers/iommu/intel-iommu.c:device_notifier() catches
> BUS_NOTIFY_REMOVED_DEVICE and removes the device from the domain (the
> domain_exit() there is only used for non-IOMMU-API domains).  It's
> possible that's the only IOMMU driver that avoids a leak due to the
> scenario you describe.  Thanks,

Thanks, that's good to know, so as a last resort I could also use the
notifier to work around the issue. But x86 seems to be the only arch
using this notifier so far, so a general fix would be nice.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/