LinuxLists.cc - [PATCH] intel-iommu: Fix use after release during device attach

2010-11-02 07:10:26

Subject: [PATCH] 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.

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(-)

diff --git a/drivers/pci/intel-iommu.c b/drivers/pci/intel-iommu.c
index 4789f8e..35463dd 100644
--- a/drivers/pci/intel-iommu.c
+++ b/drivers/pci/intel-iommu.c
@@ -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--;
}
--
1.7.1

2010-11-02 07:31:25

by Sheng Yang

[permalink] [raw]

Subject: Re: [PATCH] intel-iommu: Fix use after release during device attach

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(-)
>
> diff --git a/drivers/pci/intel-iommu.c b/drivers/pci/intel-iommu.c
> index 4789f8e..35463dd 100644
> --- a/drivers/pci/intel-iommu.c
> +++ b/drivers/pci/intel-iommu.c
> @@ -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--;
> }

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 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.

Signed-off-by: David Woodhouse <[email protected]>

diff --git a/drivers/pci/intel-iommu.c b/drivers/pci/intel-iommu.c
index c9171be..603cdc0 100644
--- a/drivers/pci/intel-iommu.c
+++ b/drivers/pci/intel-iommu.c
@@ -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

2010-11-02 07:46:22

by Jan Kiszka

[permalink] [raw]

Subject: Re: [PATCH] intel-iommu: Fix use after release during device attach

Am 02.11.2010 08:31, Sheng Yang wrote:
> 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(-)
>>
>> diff --git a/drivers/pci/intel-iommu.c b/drivers/pci/intel-iommu.c
>> index 4789f8e..35463dd 100644
>> --- a/drivers/pci/intel-iommu.c
>> +++ b/drivers/pci/intel-iommu.c
>> @@ -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--;
>> }
>
> 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 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.
>
> Signed-off-by: David Woodhouse <[email protected]>
>
> diff --git a/drivers/pci/intel-iommu.c b/drivers/pci/intel-iommu.c
> index c9171be..603cdc0 100644
> --- a/drivers/pci/intel-iommu.c
> +++ b/drivers/pci/intel-iommu.c
> @@ -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?

CONFIG_64BIT is on here, so this change did not make a difference for me.

Jan

Attachments:

signature.asc (259.00 B)
OpenPGP digital signature

2010-11-02 07:57:23

by Sheng Yang

[permalink] [raw]

Subject: Re: [PATCH] intel-iommu: Fix use after release during device attach

On Tuesday 02 November 2010 15:46:11 Jan Kiszka wrote:
> Am 02.11.2010 08:31, Sheng Yang wrote:
> > 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(-)
> >>
> >> diff --git a/drivers/pci/intel-iommu.c b/drivers/pci/intel-iommu.c
> >> index 4789f8e..35463dd 100644
> >> --- a/drivers/pci/intel-iommu.c
> >> +++ b/drivers/pci/intel-iommu.c
> >> @@ -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--;
> >>
> >> }
> >
> > 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 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.
> >
> > Signed-off-by: David Woodhouse <[email protected]>
> >
> > diff --git a/drivers/pci/intel-iommu.c b/drivers/pci/intel-iommu.c
> > index c9171be..603cdc0 100644
> > --- a/drivers/pci/intel-iommu.c
> > +++ b/drivers/pci/intel-iommu.c
> > @@ -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?
>
> CONFIG_64BIT is 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

>
> Jan

2010-11-02 08:00:43

by Sheng Yang

[permalink] [raw]

Subject: Re: [PATCH] intel-iommu: Fix use after release during device attach

On Tuesday 02 November 2010 15:31:22 Sheng Yang wrote:
> 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(-)
> >
> > diff --git a/drivers/pci/intel-iommu.c b/drivers/pci/intel-iommu.c
> > index 4789f8e..35463dd 100644
> > --- a/drivers/pci/intel-iommu.c
> > +++ b/drivers/pci/intel-iommu.c
> > @@ -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--;
> >
> > }
>
> 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 1a8bd481bfba30515b54368d90a915db3faf302f
> Author: David Woodhouse <[email protected]>
> Date: Tue Aug 10 01:38:53 2010 +0100
>
> intel-iommu: Fix 32-bit build warning with __cmpxchg()

In fact this one shouldn't affect the result. Wrong guess...

--
regards
Yang, Sheng

>
> 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]>
>
> diff --git a/drivers/pci/intel-iommu.c b/drivers/pci/intel-iommu.c
> index c9171be..603cdc0 100644
> --- a/drivers/pci/intel-iommu.c
> +++ b/drivers/pci/intel-iommu.c
> @@ -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

2010-11-14 09:18:47

by Jan Kiszka

[permalink] [raw]

Subject: Re: [PATCH] intel-iommu: Fix use after release during device attach

Attachments:

signature.asc (259.00 B)
OpenPGP digital signature

2010-12-10 08:36:36

by Jan Kiszka

[permalink] [raw]

Subject: Re: [PATCH] intel-iommu: Fix use after release during device attach

Am 14.11.2010 10:18, Jan Kiszka wrote:
> Am 02.11.2010 08:31, Sheng Yang wrote:
>> 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(-)
>>>
>>> diff --git a/drivers/pci/intel-iommu.c b/drivers/pci/intel-iommu.c
>>> index 4789f8e..35463dd 100644
>>> --- a/drivers/pci/intel-iommu.c
>>> +++ b/drivers/pci/intel-iommu.c
>>> @@ -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--;
>>> }
>>
>> Reviewed-by: Sheng Yang <[email protected]>
>>
>> CC iommu mailing list and David.
>
> Ping...
>
> I think this fix also qualifies for stable (.35 and .36).
>

Still not merged?

Jan

--
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

2010-12-10 18:44:37

by Chris Wright

[permalink] [raw]

Subject: Re: [PATCH] intel-iommu: Fix use after release during device attach

* Jan Kiszka ([email protected]) wrote:
> >>> --- a/drivers/pci/intel-iommu.c
> >>> +++ b/drivers/pci/intel-iommu.c
> >>> @@ -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));

While here, might as well remove the unnecessary cast.

> >>> + free_pgtable_page(pte);
> >>> }
> >>> dmar_domain->agaw--;
> >>> }
> >>
> >> Reviewed-by: Sheng Yang <[email protected]>

Acked-by: Chris Wright <[email protected]>

> >> 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