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 13:39:56 +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 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(), and 
iommu_attach_device_pasid(), which are only expected to be operating on 
explicitly-allocated unmanaged domains. For internal default domain 
attachment, the domain is initially derived from the device/group itself 
so we know it's appropriate by construction.

I guess this *would* now prevent some external caller reaching in and 
trying to attach something to some other group's identity default 
domain, but frankly it feels like making that fail would be no bad thing 
anyway.

Thanks,
Robin.