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.

Powered by blists - more mailing lists