linux-kernel - Re: [PATCH v5 3/7] iommu: Validate that devices match domains

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 17:05:08 +0100
From:   Robin Murphy <robin.murphy@....com>
To:     Jason Gunthorpe <jgg@...dia.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 25/10/2023 1:55 pm, Jason Gunthorpe wrote:
> 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.

It's not about one particular domain type, it's about the scope of what 
we consider valid usage. External API users should almost always be 
attaching to their own domain which they have allocated, however we also 
tolerate co-attaching additional groups to the same DMA domain in rare 
cases where it's reasonable. The fact is that those users cannot 
allocate blocking or identity domains, and I can't see that they would 
ever have any legitimate business trying to do anything with them 
anyway. So although yes, we technically lose some functionality once 
this intersects with the static domain optimisation, it's only 
questionable functionality which was never explicitly intended anyway.

I mean, what would be the valid purpose of trying to attach group A to 
group B's identity domain, even if they *were* backed by the same 
driver? At best it's pointless if group A also has its own identity 
domain already, otherwise at worst it's a deliberate attempt to 
circumvent a default domain policy imposed by the IOMMU core.

>> 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.

Sigh, back on the shelf it goes then...

Thanks,
Robin.