linux-kernel - Re: [PATCH v3 4/6] iommu: Move IOMMU pagesize check to attach

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, 20 Oct 2021 15:22:56 +0100
From:   Marc Zyngier <maz@...nel.org>
To:     Lu Baolu <baolu.lu@...ux.intel.com>
Cc:     Sven Peter <sven@...npeter.dev>, iommu@...ts.linux-foundation.org,
        Robin Murphy <robin.murphy@....com>,
        Arnd Bergmann <arnd@...nel.org>,
        Hector Martin <marcan@...can.st>, linux-kernel@...r.kernel.org,
        Alexander Graf <graf@...zon.com>,
        Mohamed Mediouni <mohamed.mediouni@...amail.com>,
        Will Deacon <will@...nel.org>,
        Alyssa Rosenzweig <alyssa@...enzweig.io>
Subject: Re: [PATCH v3 4/6] iommu: Move IOMMU pagesize check to attach_device

On Wed, 20 Oct 2021 06:21:44 +0100,
Lu Baolu <baolu.lu@...ux.intel.com> wrote:
> 
> On 2021/10/20 0:37, Sven Peter via iommu wrote:
> > The iova allocator is capable of handling any granularity which is a power
> > of two. Remove the much stronger condition that the granularity must be
> > smaller or equal to the CPU page size from a BUG_ON there.
> > Instead, check this condition during __iommu_attach_device and fail
> > gracefully.
> > 
> > Signed-off-by: Sven Peter<sven@...npeter.dev>
> > ---
> >   drivers/iommu/iommu.c | 35 ++++++++++++++++++++++++++++++++---
> >   drivers/iommu/iova.c  |  7 ++++---
> >   include/linux/iommu.h |  5 +++++
> >   3 files changed, 41 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> > index dd7863e453a5..28896739964b 100644
> > --- a/drivers/iommu/iommu.c
> > +++ b/drivers/iommu/iommu.c
> > @@ -80,6 +80,8 @@ static struct iommu_domain *__iommu_domain_alloc(struct bus_type *bus,
> >   						 unsigned type);
> >   static int __iommu_attach_device(struct iommu_domain *domain,
> >   				 struct device *dev);
> > +static void __iommu_detach_device(struct iommu_domain *domain,
> > +				  struct device *dev);
> >   static int __iommu_attach_group(struct iommu_domain *domain,
> >   				struct iommu_group *group);
> >   static void __iommu_detach_group(struct iommu_domain *domain,
> > @@ -1974,6 +1976,19 @@ void iommu_domain_free(struct iommu_domain *domain)
> >   }
> >   EXPORT_SYMBOL_GPL(iommu_domain_free);
> >   +static int iommu_check_page_size(struct iommu_domain *domain)
> > +{
> > +	if (!iommu_is_paging_domain(domain))
> > +		return 0;
> > +
> > +	if (!(domain->pgsize_bitmap & (PAGE_SIZE | (PAGE_SIZE - 1)))) {
> > +		pr_warn("IOMMU pages cannot exactly represent CPU pages.\n");
> > +		return -EFAULT;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> >   static int __iommu_attach_device(struct iommu_domain *domain,
> >   				 struct device *dev)
> >   {
> > @@ -1983,9 +1998,23 @@ static int __iommu_attach_device(struct iommu_domain *domain,
> >   		return -ENODEV;
> >     	ret = domain->ops->attach_dev(domain, dev);
> > -	if (!ret)
> > -		trace_attach_device_to_domain(dev);
> > -	return ret;
> > +	if (ret)
> > +		return ret;
> > +
> > +	/*
> > +	 * Check that CPU pages can be represented by the IOVA granularity.
> > +	 * This has to be done after ops->attach_dev since many IOMMU drivers
> > +	 * only limit domain->pgsize_bitmap after having attached the first
> > +	 * device.
> > +	 */
> > +	ret = iommu_check_page_size(domain);
> > +	if (ret) {
> > +		__iommu_detach_device(domain, dev);
> > +		return ret;
> > +	}
> 
> It looks odd. __iommu_attach_device() attaches an I/O page table for a
> device. How does it relate to CPU pages? Why is it a failure case if CPU
> page size is not covered?

If you allocate a CPU PAGE_SIZE'd region, and point it at a device
that now can DMA to more than what you have allocated because the
IOMMU's own page size is larger, the device has now access to data it
shouldn't see. In my book, that's a pretty bad thing.

	M.

-- 
Without deviation from the norm, progress is not possible.