Re: [PATCH] iommu/iova: validate iova_domain input to put_iova_domain

[nwatters]

On 2016-07-14 07:21, Auger Eric wrote:
> Hi Robin, Nate,
> On 14/07/2016 12:36, Robin Murphy wrote:
> > On 14/07/16 09:34, Joerg Roedel wrote:
> > > On Wed, Jul 13, 2016 at 02:49:32PM -0400, Nate Watterson wrote:
> > > > Passing a NULL or uninitialized iova_domain into put_iova_domain
> > > > will currently crash the kernel when the unconfigured iova_domain
> > > > data members are accessed. To prevent this from occurring, this 
> > > > patch
> > > > adds a check to make sure that the domain is non-NULL and that the
> > > > domain granule is non-zero. The granule can be used to check if the
> > > > domain was properly initialized because calling init_iova_domain
> > > > with a granule of zero would have already triggered a BUG statement
> > > > crashing the kernel.
> > >
> > > Have you seen real crashes happening because of this?

In my case, it was calling iommu_request_dm_for_dev() which triggered 
the
"iommu_[get/put]_dma_cookie() without iommu_dma_init_domain()" issue 
that
has Robin documented below.

> I also saw the crash happening with my PCIe passthrough series (not
> upstreamed)
> [PATCH v10 0/8] [PATCH v10 0/8] KVM PCIe/MSI passthrough on ARM/ARM64:
> kernel part 1/3: iommu changes  https://lkml.org/lkml/2016/6/7/676
>
> patch [PATCH v10 8/8] iommu/arm-smmu: get/put the msi cookie
> also uses iommu_put_dma_cookie
>
>
> and the uninitialised lock crash happens if the group gets destroyed
> before the iommu_dma_init_domain is called, which can also happen for 
> me.
>
> > It _can_ happen via the iommu-dma code if something goes wrong
> > initialising a group - the IOVA domain gets allocated at the same time
> > as the default IOMMU domain, but isn't initialised until later once 
> > the
> > device in question gets ity dma ops set up. If adding the device to 
> > the
> > group fails, everything gets torn down again and 
> > iommu_put_dma_cookie()
> > ends up trying to take an uninitialised lock .
> Cant' we allow the granule check also with UNMANAGED type?
>
> Thanks
>
> Eric
>
> > However, I think the appropriate fix for that particular situation 
> > would
> > be more like this:
> >
> > diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
> > index ea5a9ebf0f78..d00d22930a6b 100644
> > --- a/drivers/iommu/dma-iommu.c
> > +++ b/drivers/iommu/dma-iommu.c
> > @@ -65,10 +65,11 @@ void iommu_put_dma_cookie(struct iommu_domain 
> > *domain)
> >  {
> >         struct iova_domain *iovad = domain->iova_cookie;
> >
> > -       if (!iovad)
> > +       if (domain->type != IOMMU_DOMAIN_DMA || !iovad)
> >                 return;
> >
> > -       put_iova_domain(iovad);
> > +       if (iovad->granule)
> > +               put_iova_domain(iovad);
> >         kfree(iovad);
> >         domain->iova_cookie = NULL;
> >  }
> >
> > (It probably should have been that way from the start; mea culpa)

I originally put together a similar patch, but then thought that people 
would
complain it didn't fix the root of the problem. Yet another instance 
where
thinking was best avoided I guess.

> > Robin.
> > _______________________________________________
> > iommu mailing list
> > iommu@xxxxxxxxxxxxxxxxxxxxxxxxxx
> > https://lists.linuxfoundation.org/mailman/listinfo/iommu

--
Qualcomm Datacenter Technologies, Inc. on behalf of the Qualcomm 
Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a 
Linux Foundation Collaborative Project.