linux-kernel - Re: [PATCH 1/2] vfio/type1: Simplify bus

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20220610000343.GD1343366@nvidia.com>
Date:   Thu, 9 Jun 2022 21:03:43 -0300
From:   Jason Gunthorpe <jgg@...dia.com>
To:     Robin Murphy <robin.murphy@....com>
Cc:     alex.williamson@...hat.com, cohuck@...hat.com, kvm@...r.kernel.org,
        iommu@...ts.linux-foundation.org, linux-kernel@...r.kernel.org,
        baolu.lu@...ux.intel.com
Subject: Re: [PATCH 1/2] vfio/type1: Simplify bus_type determination

On Wed, Jun 08, 2022 at 03:25:49PM +0100, Robin Murphy wrote:
> Since IOMMU groups are mandatory for drivers to support, it stands to
> reason that any device which has been successfully be added to a group
> must be on a bus supported by that IOMMU driver, and therefore a domain
> viable for any device in the group must be viable for all devices in
> the group. This already has to be the case for the IOMMU API's internal
> default domain, for instance. Thus even if the group contains devices
> on different buses, that can only mean that the IOMMU driver actually
> supports such an odd topology, and so without loss of generality we can
> expect the bus type of any arbitrary device in a group to be suitable
> for IOMMU API calls.
> 
> Replace vfio_bus_type() with a trivial callback that simply returns any
> device from which to then derive a usable bus type. This is also a step
> towards removing the vague bus-based interfaces from the IOMMU API.
> 
> Furthermore, scrutiny reveals a lack of protection for the bus and/or
> device being removed while .attach_group is inspecting them; the
> reference we hold on the iommu_group ensures that data remains valid,
> but does not prevent the group's membership changing underfoot. Holding
> the vfio_goup's device_lock should be sufficient to block any relevant
> device's VFIO driver from unregistering, and thus block unbinding and
> any further stages of removal for the duration of the attach operation.

The device_lock only protects devices that are on the device_list from
concurrent unregistration, the device returned by
iommu_group_for_each_dev() is not guarented to be the on the device
list.

> @@ -760,8 +760,11 @@ static int __vfio_container_attach_groups(struct vfio_container *container,
>  	int ret = -ENODEV;
>  
>  	list_for_each_entry(group, &container->group_list, container_next) {
> +		/* Prevent devices unregistering during attach */
> +		mutex_lock(&group->device_lock);
>  		ret = driver->ops->attach_group(data, group->iommu_group,
>  						group->type);
> +		mutex_unlock(&group->device_lock);

I still prefer the version where we pass in an arbitrary vfio_device
from the list the group maintains:

   list_first_entry(group->device_list)

And don't call iommu_group_for_each_dev(), it is much simpler to
reason about how it works.

Jason