iommu/iova: validate iova_domain input to put_iova_domain
Subsystems (2)
PATCH 1/1 iommu/iova: validate iova_domain input to put_iova_domain
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.
--- drivers/iommu/iova.c | 4 ++++ 1 file changed, 4 insertions(+)
drivers/iommu/iova.c +4 -0
@@ -459,6 +459,10 @@ void put_iova_domain(struct iova_domain *iovad)struct rb_node *node;unsigned long flags; +/* Only teardown properly initialized domains */ +if (!iovad || !iovad->granule) +return ; +free_iova_rcaches (iovad);spin_lock_irqsave (&iovad->iova_rbtree_lock, flags); node =rb_first (&iovad->rbroot);
Signature · 3 lines
Qualcomm Datacenter Technologies, Inc. on behalf of Qualcomm Technologies, Inc. Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.
Joerg Roedel
maintainerQuoted 8 lines · Nate Watterson
Passing a NULL or uninitialized
iova_domainintoput_iova_domainwill currently crash the kernel when the unconfigurediova_domaindata 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 callinginit_iova_domainwith a granule of zero would have already triggered a BUG statement crashing the kernel.
Have you seen real crashes happening because of this?
Joerg
Robin Murphy
maintainerQuoted 11 lines · Joerg Roedel
Quoted 8 lines · Nate Watterson
Passing a NULL or uninitialized
iova_domainintoput_iova_domainwill currently crash the kernel when the unconfigurediova_domaindata 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 callinginit_iova_domainwith a granule of zero would have already triggered a BUG statement crashing the kernel.Have you seen real crashes happening because of this?
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 .
However, I think the appropriate fix for that particular situation would be more like this:
drivers/iommu/dma-iommu.c +3 -2
@@ -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)
Robin.
Hi Robin, Nate,
Quoted 11 lines · Robin Murphy
Quoted 10 lines · Joerg Roedel
Quoted 8 lines · Nate Watterson
Passing a NULL or uninitialized
iova_domainintoput_iova_domainwill currently crash the kernel when the unconfigurediova_domaindata 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 callinginit_iova_domainwith a granule of zero would have already triggered a BUG statement crashing the kernel.
Quoted 1 line
Quoted 1 line
Have you seen real crashes happening because of this?
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.
Quoted 7 lines
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
Quoted 31 lines
However, I think the appropriate fix for that particular situation would be more like this:
drivers/iommu/dma-iommu.c +3
-2@@ -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)
Robin.
_______________________________________________
iommu mailing list iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Quoted 14 lines · Auger Eric
Hi Robin, Nate,
Quoted 12 lines · Robin Murphy
Quoted 11 lines · Joerg Roedel
Quoted 8 lines · Nate Watterson
Passing a NULL or uninitialized
iova_domainintoput_iova_domainwill currently crash the kernel when the unconfigurediova_domaindata 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 callinginit_iova_domainwith 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.
Quoted 43 lines
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/676patch [PATCH v10 8/8] iommu/arm-smmu: get/put the msi cookie also uses
iommu_put_dma_cookieand the uninitialised lock crash happens if the group gets destroyed before the
iommu_dma_init_domainis called, which can also happen for me.Quoted 7 lines
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
Quoted 17 lines
However, I think the appropriate fix for that particular situation would be more like this:
drivers/iommu/dma-iommu.c +1
-1@@ -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 ;
Quoted 8 lines
Quoted 8 lines
drivers/iommu/dma-iommu.c +2
-1-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.
Quoted 7 lines
Quoted 7 lines
Robin.
_______________________________________________
iommu mailing list iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Signature · 2 lines
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.