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]
Date:   Wed, 25 Oct 2023 09:55:28 -0300
From:   Jason Gunthorpe <jgg@...dia.com>
To:     Robin Murphy <robin.murphy@....com>
Cc:     joro@...tes.org, will@...nel.org, iommu@...ts.linux.dev,
        baolu.lu@...ux.intel.com, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v5 3/7] iommu: Validate that devices match domains

On Wed, Oct 25, 2023 at 01:39:56PM +0100, Robin Murphy wrote:
> On 24/10/2023 7:52 pm, Jason Gunthorpe wrote:
> > On Wed, Oct 11, 2023 at 07:14:50PM +0100, Robin Murphy wrote:
> > 
> > > @@ -2279,10 +2280,16 @@ struct iommu_domain *iommu_get_dma_domain(struct device *dev)
> > >   static int __iommu_attach_group(struct iommu_domain *domain,
> > >   				struct iommu_group *group)
> > >   {
> > > +	struct device *dev;
> > > +
> > >   	if (group->domain && group->domain != group->default_domain &&
> > >   	    group->domain != group->blocking_domain)
> > >   		return -EBUSY;
> > > +	dev = iommu_group_first_dev(group);
> > > +	if (!dev_has_iommu(dev) || dev_iommu_ops(dev) != domain->owner)
> > > +		return -EINVAL;
> > 
> > I was thinking about this later, how does this work for the global
> > static domains? domain->owner will not be set?
> > 
> > 	if (alloc_type == IOMMU_DOMAIN_IDENTITY && ops->identity_domain)
> > 		return ops->identity_domain;
> > 	else if (alloc_type == IOMMU_DOMAIN_BLOCKED && ops->blocked_domain)
> > 		return ops->blocked_domain;
> > 
> > Seems like it will break everything?
> 
> I don't believe it makes any significant difference - as the commit message
> points out, this validation is only applied at the public interface
> boundaries of iommu_attach_group(), iommu_attach_device(), 

Oh, making it only work for on domain type seems kind of hacky..

If that is the intention maybe the owner set should be moved into
iommu_domain_alloc() with a little comment noting that it is limited
to work in only a few cases?

I certainly didn't understand from the commit message to mean it was
only actually working for one domain type and this also blocks using
other types with the public interface.

> and iommu_attach_device_pasid(), which are only expected to be
> operating on explicitly-allocated unmanaged domains.

We have nesting now in the iommufd branch, and SVA will come soon for
these APIs.

Regardless this will clash with the iommufd branch for this reason so
I guess it needs to wait till rc1.

Thanks,
Jason

Powered by blists - more mailing lists