intel-iommu: Fix use after release during device attach
PATCH 1/1 intel-iommu: Fix use after release during device attach
From: Jan Kiszka <[email protected]>
Obtail the new pgd pointer before releasing the page containing this value.
---
Who is taking care of this? The kvm tree?
drivers/pci/intel-iommu.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)
drivers/pci/intel-iommu.c +1 -1
@@ -3627,9 +3627,9 @@ static int intel_iommu_attach_device(struct iommu_domain *domain, pte = dmar_domain->pgd;if (dma_pte_present (pte)) { -free_pgtable_page (dmar_domain->pgd); dmar_domain->pgd = (struct dma_pte *)phys_to_virt (dma_pte_addr (pte)); +free_pgtable_page (pte); } dmar_domain->agaw--; }
Signature · 1 line
1.7.1
Quoted 28 lines · On Tuesday 02 November 2010 15:05:51 Jan Kiszka wrote:
From: Jan Kiszka <[email protected]>
Obtail the new pgd pointer before releasing the page containing this value.
Signed-off-by Jan Kiszka <[email protected]>---
Who is taking care of this? The kvm tree?
drivers/pci/intel-iommu.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)drivers/pci/intel-iommu.c +1
-1@@ -3627,9 +3627,9 @@ static int intel_iommu_attach_device(structiommu_domain *domain, pte = dmar_domain->pgd;if (dma_pte_present (pte)) { -free_pgtable_page (dmar_domain->pgd); dmar_domain->pgd = (struct dma_pte *)phys_to_virt (dma_pte_addr (pte)); +free_pgtable_page (pte); } dmar_domain->agaw--;
Quoted 1 line
}
CC iommu mailing list and David.
OK, Jan, I got your meaning now. And it's not the exactly swap. :)
I think the old code is safe, seems it's broken(exposed) by:
commit 1a8bd481bfba30515b54368d90a915db3faf302f
Author: David Woodhouse <[email protected]> Date: Tue Aug 10 01:38:53 2010 +0100
intel-iommu: Fix 32-bit build warning with __cmpxchg()
drivers/pci/intel-iommu.c: In function 'dma_pte_addr':
drivers/pci/intel-iommu.c:239: warning: passing argument 1 of '__cmpxchg64'
from incompatible pointer typ
It seems that __cmpxchg64() now cares about the type of its pointer argument, so give it a (uint64_t *) instead of a pointer to a structure which contains only that.
drivers/pci/intel-iommu.c +1 -1
@@ -236,7 +236,7 @@ static inline u64 dma_pte_addr(struct dma_pte *pte)return pte->val &VTD_PAGE_MASK ;#else /* Must have a full atomic 64-bit read */ -return __cmpxchg64 (pte,0ULL ,0ULL ) &VTD_PAGE_MASK ; +return __cmpxchg64 (&pte->val,0ULL ,0ULL ) &VTD_PAGE_MASK ;#endif }
Seems here is the only affected code?
--
regards
Yang, Sheng
Quoted 68 lines · Sheng Yang
Quoted 28 lines · On Tuesday 02 November 2010 15:05:51 Jan Kiszka wrote:
From: Jan Kiszka <[email protected]>
Obtail the new pgd pointer before releasing the page containing this value.
Signed-off-by Jan Kiszka <[email protected]>---
Who is taking care of this? The kvm tree?
drivers/pci/intel-iommu.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)drivers/pci/intel-iommu.c +1
-1@@ -3627,9 +3627,9 @@ static int intel_iommu_attach_device(structiommu_domain *domain, pte = dmar_domain->pgd;if (dma_pte_present (pte)) { -free_pgtable_page (dmar_domain->pgd); dmar_domain->pgd = (struct dma_pte *)phys_to_virt (dma_pte_addr (pte)); +free_pgtable_page (pte); } dmar_domain->agaw--;Quoted 1 line
}
Reviewed-by Sheng Yang <[email protected]>CC iommu mailing list and David.
OK, Jan, I got your meaning now. And it's not the exactly swap. :)
I think the old code is safe, seems it's broken(exposed) by:
commit
1a8bd481bfba30515b54368d90a915db3faf302fAuthor: David Woodhouse <[email protected]> Date: Tue Aug 10 01:38:53 2010 +0100intel-iommu: Fix 32-bit build warning with
__cmpxchg()
drivers/pci/intel-iommu.c: In function 'dma_pte_addr':
drivers/pci/intel-iommu.c:239: warning: passing argument 1 of '__cmpxchg64'
from incompatible pointer typIt seems that
__cmpxchg64()now cares about the type of its pointer argument, so give it a (uint64_t*) instead of a pointer to a structure which contains only that.Signed-off-by David Woodhouse <[email protected]>drivers/pci/intel-iommu.c +1
-1@@ -236,7 +236,7 @@ static inline u64 dma_pte_addr(struct dma_pte *pte)return pte->val &VTD_PAGE_MASK ;#else /* Must have a full atomic 64-bit read */ -return __cmpxchg64 (pte,0ULL ,0ULL ) &VTD_PAGE_MASK ; +return __cmpxchg64 (&pte->val,0ULL ,0ULL ) &VTD_PAGE_MASK ;#endif }
Quoted 1 line
Seems here is the only affected code?
CONFIG_64BIT is on here, so this change did not make a difference for me.
Jan
Quoted 79 lines · On Tuesday 02 November 2010 15:46:11 Jan Kiszka wrote:
Quoted 78 lines · Sheng Yang
Quoted 25 lines · On Tuesday 02 November 2010 15:05:51 Jan Kiszka wrote:
From: Jan Kiszka <[email protected]>
Obtail the new pgd pointer before releasing the page containing this value.
Signed-off-by Jan Kiszka <[email protected]>---
Who is taking care of this? The kvm tree?
drivers/pci/intel-iommu.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)drivers/pci/intel-iommu.c +0
-1@@ -3627,9 +3627,9 @@ static int intel_iommu_attach_device(structiommu_domain *domain, pte = dmar_domain->pgd;if (dma_pte_present (pte)) { -free_pgtable_page (dmar_domain->pgd);drivers/pci/intel-iommu.c static int intel_iommu_attach_device(struct
Quoted 10 lines
drivers/pci/intel-iommu.c +1
-0dmar_domain->pgd = (struct dma_pte *)phys_to_virt (dma_pte_addr (pte)); +free_pgtable_page (pte); } dmar_domain->agaw--; }Reviewed-by Sheng Yang <[email protected]>CC iommu mailing list and David.
OK, Jan, I got your meaning now. And it's not the exactly swap. :)
I think the old code is safe, seems it's broken(exposed) by:
commit
1a8bd481bfba30515b54368d90a915db3faf302fAuthor: David Woodhouse <[email protected]> Date: Tue Aug 10 01:38:53 2010 +0100intel-iommu: Fix 32-bit build warning with
__cmpxchg()
drivers/pci/intel-iommu.c: In function 'dma_pte_addr':
drivers/pci/intel-iommu.c:239: warning: passing argument 1 of '__cmpxchg64'from incompatible pointer typ
It seems that
__cmpxchg64()now cares about the type of its pointer argument, so give it a (uint64_t*) instead of a pointer to a structure which contains only that.Signed-off-by David Woodhouse <[email protected]>drivers/pci/intel-iommu.c +1
-1@@ -236,7 +236,7 @@ static inline u64 dma_pte_addr(struct dma_pte *pte)return pte->val &VTD_PAGE_MASK ;#else /* Must have a full atomic 64-bit read */ -return __cmpxchg64 (pte,0ULL ,0ULL ) &VTD_PAGE_MASK ; +return __cmpxchg64 (&pte->val,0ULL ,0ULL ) &VTD_PAGE_MASK ;
Quoted 6 lines
Quoted 4 lines
#endif
}Seems here is the only affected code?
CONFIG_64BITis on here, so this change did not make a difference for me.
Oh...
Then it would due to most VT-d machine wouldn't run into while (iommu->agaw < dmar_domain->agaw).
We have routing test for VT-d devices assignment, but seems we don't use this kind of VT-d machine for testing.
--
regards
Yang, Sheng
Quoted 2 lines
Jan
Quoted 43 lines · On Tuesday 02 November 2010 15:31:22 Sheng Yang wrote:
Quoted 25 lines · On Tuesday 02 November 2010 15:05:51 Jan Kiszka wrote:
From: Jan Kiszka <[email protected]>
Obtail the new pgd pointer before releasing the page containing this value.
Signed-off-by Jan Kiszka <[email protected]>---
Who is taking care of this? The kvm tree?
drivers/pci/intel-iommu.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)drivers/pci/intel-iommu.c +0
-1@@ -3627,9 +3627,9 @@ static int intel_iommu_attach_device(structiommu_domain *domain, pte = dmar_domain->pgd;if (dma_pte_present (pte)) { -free_pgtable_page (dmar_domain->pgd);drivers/pci/intel-iommu.c static int intel_iommu_attach_device(struct
Quoted 10 lines
drivers/pci/intel-iommu.c +1
-0dmar_domain->pgd = (struct dma_pte *)phys_to_virt (dma_pte_addr (pte)); +free_pgtable_page (pte); } dmar_domain->agaw--; }Reviewed-by Sheng Yang <[email protected]>CC iommu mailing list and David.
OK, Jan, I got your meaning now. And it's not the exactly swap. :)
Quoted 7 lines
I think the old code is safe, seems it's broken(exposed) by:
commit
1a8bd481bfba30515b54368d90a915db3faf302fAuthor: David Woodhouse <[email protected]> Date: Tue Aug 10 01:38:53 2010 +0100intel-iommu: Fix 32-bit build warning with
__cmpxchg()
In fact this one shouldn't affect the result. Wrong guess...
--
regards
Yang, Sheng
Quoted 33 lines
drivers/pci/intel-iommu.c: In function 'dma_pte_addr':
drivers/pci/intel-iommu.c:239: warning: passing argument 1 of
'__cmpxchg64' from incompatible pointer typIt seems that
__cmpxchg64()now cares about the type of its pointer
argument, so give it a (uint64_t*) instead of a pointer to a structure which contains only that.Signed-off-by David Woodhouse <[email protected]>drivers/pci/intel-iommu.c +1
-1@@ -236,7 +236,7 @@ static inline u64 dma_pte_addr(struct dma_pte *pte)return pte->val &VTD_PAGE_MASK ;#else /* Must have a full atomic 64-bit read */ -return __cmpxchg64 (pte,0ULL ,0ULL ) &VTD_PAGE_MASK ; +return __cmpxchg64 (&pte->val,0ULL ,0ULL ) &VTD_PAGE_MASK ;#endif }Seems here is the only affected code?
--
regards
Yang, Sheng
--
To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Quoted 33 lines · Sheng Yang
Quoted 28 lines · On Tuesday 02 November 2010 15:05:51 Jan Kiszka wrote:
From: Jan Kiszka <[email protected]>
Obtail the new pgd pointer before releasing the page containing this value.
Signed-off-by Jan Kiszka <[email protected]>---
Who is taking care of this? The kvm tree?
drivers/pci/intel-iommu.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)drivers/pci/intel-iommu.c +1
-1@@ -3627,9 +3627,9 @@ static int intel_iommu_attach_device(structiommu_domain *domain, pte = dmar_domain->pgd;if (dma_pte_present (pte)) { -free_pgtable_page (dmar_domain->pgd); dmar_domain->pgd = (struct dma_pte *)phys_to_virt (dma_pte_addr (pte)); +free_pgtable_page (pte); } dmar_domain->agaw--;Quoted 1 line
}
Reviewed-by Sheng Yang <[email protected]>
Quoted 1 line
CC iommu mailing list and David.
Ping...
I think this fix also qualifies for stable (.35 and .36).
Jan
Quoted 34 lines · Jan Kiszka
Quoted 33 lines · Sheng Yang
Quoted 28 lines · On Tuesday 02 November 2010 15:05:51 Jan Kiszka wrote:
From: Jan Kiszka <[email protected]>
Obtail the new pgd pointer before releasing the page containing this value.
Signed-off-by Jan Kiszka <[email protected]>---
Who is taking care of this? The kvm tree?
drivers/pci/intel-iommu.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)drivers/pci/intel-iommu.c +1
-1@@ -3627,9 +3627,9 @@ static int intel_iommu_attach_device(structiommu_domain *domain, pte = dmar_domain->pgd;if (dma_pte_present (pte)) { -free_pgtable_page (dmar_domain->pgd); dmar_domain->pgd = (struct dma_pte *)phys_to_virt (dma_pte_addr (pte)); +free_pgtable_page (pte); } dmar_domain->agaw--;Quoted 1 line
}
Reviewed-by Sheng Yang <[email protected]>
Quoted 6 lines
Quoted 1 line
CC iommu mailing list and David.
Ping...
I think this fix also qualifies for stable (.35 and .36).
Still not merged?
Jan
Signature · 2 lines
Siemens AG, Corporate Technology, CT T DE IT 1 Corporate Competence Center Embedded Linux
Chris Wright
Acked-byQuoted 10 lines · * Jan Kiszka ([email protected]) wrote:
Quoted 10 lines
Quoted 10 lines
drivers/pci/intel-iommu.c static int intel_iommu_attach_device(struct · line 3632
Quoted 10 lines
drivers/pci/intel-iommu.c +0
-1@@ -3627,9 +3627,9 @@ static int intel_iommu_attach_device(structiommu_domain *domain, pte = dmar_domain->pgd;if (dma_pte_present (pte)) { -free_pgtable_page (dmar_domain->pgd); dmar_domain->pgd = (struct dma_pte *)phys_to_virt (dma_pte_addr (pte));
While here, might as well remove the unnecessary cast.
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Quoted 4 lines
drivers/pci/intel-iommu.c +1
-0+free_pgtable_page (pte); } dmar_domain->agaw--;}
Reviewed-by Sheng Yang <[email protected]>
Quoted 8 lines
Quoted 6 lines
Quoted 1 line
CC iommu mailing list and David.
Ping...
I think this fix also qualifies for stable (.35 and .36).
Still not merged?
David, do you plan to pick this one up?
thanks,
-chris
Quoted 30 lines · Chris Wright
Quoted 10 lines · * Jan Kiszka ([email protected]) wrote:
Quoted 10 lines
Quoted 10 lines
drivers/pci/intel-iommu.c static int intel_iommu_attach_device(struct · line 3632
Quoted 10 lines
drivers/pci/intel-iommu.c +0
-1@@ -3627,9 +3627,9 @@ static int intel_iommu_attach_device(structiommu_domain *domain, pte = dmar_domain->pgd;if (dma_pte_present (pte)) { -free_pgtable_page (dmar_domain->pgd); dmar_domain->pgd = (struct dma_pte *)phys_to_virt (dma_pte_addr (pte));While here, might as well remove the unnecessary cast.
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Quoted 4 lines
drivers/pci/intel-iommu.c +1
-0+free_pgtable_page (pte); } dmar_domain->agaw--;}
Reviewed-by Sheng Yang <[email protected]>Acked-by Chris Wright <[email protected]>Quoted 7 lines
Quoted 6 lines
Quoted 1 line
CC iommu mailing list and David.
Ping...
I think this fix also qualifies for stable (.35 and .36).
Quoted 6 lines
Quoted 1 line
Still not merged?
David, do you plan to pick this one up?
thanks,
-chris
Hmm, still no reaction. Trying David's Intel address now...
Jan
PATCH v2 1/1 intel-iommu: Fix use after release during device attach
Quoted 33 lines · Jan Kiszka
Quoted 32 lines · Chris Wright
Quoted 10 lines · * Jan Kiszka ([email protected]) wrote:
Quoted 10 lines
Quoted 10 lines
drivers/pci/intel-iommu.c static int intel_iommu_attach_device(struct · line 3632
Quoted 10 lines
drivers/pci/intel-iommu.c +0
-1@@ -3627,9 +3627,9 @@ static int intel_iommu_attach_device(structiommu_domain *domain, pte = dmar_domain->pgd;if (dma_pte_present (pte)) { -free_pgtable_page (dmar_domain->pgd); dmar_domain->pgd = (struct dma_pte *)phys_to_virt (dma_pte_addr (pte));While here, might as well remove the unnecessary cast.
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Quoted 4 lines
drivers/pci/intel-iommu.c +1
-0+free_pgtable_page (pte); } dmar_domain->agaw--;}
Reviewed-by Sheng Yang <[email protected]>Acked-by Chris Wright <[email protected]>Quoted 8 lines
Quoted 6 lines
Quoted 1 line
CC iommu mailing list and David.
Ping...
I think this fix also qualifies for stable (.35 and .36).
Still not merged?
Quoted 9 lines
Quoted 4 lines
David, do you plan to pick this one up?
thanks,
-chrisHmm, still no reaction. Trying David's Intel address now...
Jan
Walking through my old queues, I came across this one again.
Given the still lacking reaction from the official maintainer, I'm a bit confused about the state of intel-iommu. Is it unmaintained? Should this bug fix better be routed through the KVM tree as its only in-tree user? Please enlighten me.
Note that the patch became stable material for 35..38 in the meantime, and it should go into 39 before release as well.
Thanks,
Jan
-------8<--------
Obtain the new pgd pointer before releasing the page containing this value. Remove unneeded cast at this chance as well.
--- drivers/pci/intel-iommu.c | 5 ++--- 1 files changed, 2 insertions(+), 3 deletions(-)
v1->v2: Clean up cast as suggested by Chris.
drivers/pci/intel-iommu.c +2 -3
@@ -3607,9 +3607,8 @@ static int intel_iommu_attach_device(struct iommu_domain *domain, pte = dmar_domain->pgd;if (dma_pte_present (pte)) { -free_pgtable_page (dmar_domain->pgd); - dmar_domain->pgd = (struct dma_pte *) -phys_to_virt (dma_pte_addr (pte)); + dmar_domain->pgd =phys_to_virt (dma_pte_addr (pte)); +free_pgtable_page (pte); } dmar_domain->agaw--; }
Signature · 1 line
1.7.1
Quoted 57 lines · * Jan Kiszka ([email protected]) wrote:
Quoted 33 lines · Jan Kiszka
Quoted 32 lines · Chris Wright
Quoted 10 lines · * Jan Kiszka ([email protected]) wrote:
Quoted 10 lines
Quoted 10 lines
drivers/pci/intel-iommu.c static int intel_iommu_attach_device(struct · line 3632
Quoted 10 lines
drivers/pci/intel-iommu.c +0
-1@@ -3627,9 +3627,9 @@ static int intel_iommu_attach_device(structiommu_domain *domain, pte = dmar_domain->pgd;if (dma_pte_present (pte)) { -free_pgtable_page (dmar_domain->pgd); dmar_domain->pgd = (struct dma_pte *)phys_to_virt (dma_pte_addr (pte));While here, might as well remove the unnecessary cast.
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Quoted 4 lines
drivers/pci/intel-iommu.c +1
-0+free_pgtable_page (pte); } dmar_domain->agaw--;}
Reviewed-by Sheng Yang <[email protected]>Acked-by Chris Wright <[email protected]>Quoted 8 lines
Quoted 6 lines
Quoted 1 line
CC iommu mailing list and David.
Ping...
I think this fix also qualifies for stable (.35 and .36).
Still not merged?
Quoted 9 lines
Quoted 4 lines
David, do you plan to pick this one up?
thanks,
-chrisHmm, still no reaction. Trying David's Intel address now...
Jan
Walking through my old queues, I came across this one again.
Given the still lacking reaction from the official maintainer, I'm a bit confused about the state of intel-iommu. Is it unmaintained? Should this bug fix better be routed through the KVM tree as its only in-tree user? Please enlighten me.
Note that the patch became stable material for 35..38 in the meantime, and it should go into 39 before release as well.
Thanks,
Jan
Quoted 6 lines
-------8<--------
Obtain the new pgd pointer before releasing the page containing this value. Remove unneeded cast at this chance as well.
Signed-off-by Jan Kiszka <[email protected]>
Quoted 22 lines
--- drivers/pci/intel-iommu.c | 5 ++--- 1 files changed, 2 insertions(+), 3 deletions(-)
v1->v2: Clean up cast as suggested by Chris.drivers/pci/intel-iommu.c +2
-3@@ -3607,9 +3607,8 @@ static int intel_iommu_attach_device(struct iommu_domain *domain, pte = dmar_domain->pgd;if (dma_pte_present (pte)) { -free_pgtable_page (dmar_domain->pgd); - dmar_domain->pgd = (struct dma_pte *) -phys_to_virt (dma_pte_addr (pte)); + dmar_domain->pgd =phys_to_virt (dma_pte_addr (pte)); +free_pgtable_page (pte); } dmar_domain->agaw--; }
Quoted 44 lines · Jan Kiszka
Quoted 33 lines · Jan Kiszka
Quoted 32 lines · Chris Wright
Quoted 10 lines · * Jan Kiszka ([email protected]) wrote:
Quoted 10 lines
Quoted 10 lines
drivers/pci/intel-iommu.c static int intel_iommu_attach_device(struct · line 3632
Quoted 10 lines
drivers/pci/intel-iommu.c +0
-1@@ -3627,9 +3627,9 @@ static int intel_iommu_attach_device(structiommu_domain *domain, pte = dmar_domain->pgd;if (dma_pte_present (pte)) { -free_pgtable_page (dmar_domain->pgd); dmar_domain->pgd = (struct dma_pte *)phys_to_virt (dma_pte_addr (pte));While here, might as well remove the unnecessary cast.
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Quoted 4 lines
drivers/pci/intel-iommu.c +1
-0+free_pgtable_page (pte); } dmar_domain->agaw--;}
Reviewed-by Sheng Yang <[email protected]>Acked-by Chris Wright <[email protected]>Quoted 8 lines
Quoted 6 lines
Quoted 1 line
CC iommu mailing list and David.
Ping...
I think this fix also qualifies for stable (.35 and .36).
Still not merged?
Quoted 9 lines
Quoted 4 lines
David, do you plan to pick this one up?
thanks,
-chrisHmm, still no reaction. Trying David's Intel address now...
Jan
Quoted 6 lines
Walking through my old queues, I came across this one again.
Given the still lacking reaction from the official maintainer, I'm a bit confused about the state of intel-iommu. Is it unmaintained? Should this bug fix better be routed through the KVM tree as its only in-tree user? Please enlighten me.
I've been wondering the exact same thing. My last patch took weeks of prodding, finally went into the maintainer's tree without acknowledgment, and there's hardly been any activity there to suggest a pull request for 2.6.39 is going to happen. David, are you still interested in maintaining this code? Thanks,
Alex
Quoted 5 lines · Alex Williamson
I've been wondering the exact same thing. My last patch took weeks of prodding, finally went into the maintainer's tree without acknowledgment, and there's hardly been any activity there to suggest a pull request for 2.6.39 is going to happen. David, are you still interested in maintaining this code? Thanks,
Yes, sorry, I've been somewhat snowed under with various things.
This patch has been in my tree for a while, and I've just merged one more patch which is outstanding and sent Linus a pull request.
Signature · 1 line
dwmw2
Quoted 11 lines · David Woodhouse
Quoted 5 lines · Alex Williamson
I've been wondering the exact same thing. My last patch took weeks of prodding, finally went into the maintainer's tree without acknowledgment, and there's hardly been any activity there to suggest a pull request for 2.6.39 is going to happen. David, are you still interested in maintaining this code? Thanks,
Yes, sorry, I've been somewhat snowed under with various things.
This patch has been in my tree for a while, and I've just merged one more patch which is outstanding and sent Linus a pull request.
Thanks David, I know you've been busy lately.
Alex