Re: [Xen-devel] [PATCH] xen: avoid writing to freed memory after race in p2m handling

2014-10-14 Thread David Vrabel 
On 14/10/14 10:49, Juergen Gross wrote:
> On 10/14/2014 11:30 AM, David Vrabel wrote:
>> On 14/10/14 10:00, Juergen Gross wrote:
>>> In case a race was detected during allocation of a new p2m tree
>>> element in alloc_p2m() the new allocated mid_mfn page is freed without
>>> updating the pointer to the found value in the tree. This will result
>>> in overwriting the just freed page with the mfn of the p2m leaf.
>>
>> Can this race actually happen?  i.e., does this need tagging for stable?
> 
> Good question. I just stumbled over it while writing the linear p2m-list
> patch.
> 
> Is it possible for gnttab_map_refs() to call set_foreign_p2m_mapping()
> specifying a pfn which has been invalid before? In this case the race
> could happen in dom0.

Yes, if two backends map into ballooned pages from a region of
untouched,  pre-ballooned memory.  But these seems super rare and I
don't think there have been any bug reports that could be attributed to
this, so I don't think a stable backport is needed.

> I think ballooning alone can't trigger this race, as it is calling
> set_phys_to_machine() under lock only.

Agreed.

Applied to stable/for-linus-3.18.

Thanks.

David
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [Xen-devel] [PATCH] xen: avoid writing to freed memory after race in p2m handling

2014-10-14 Thread Juergen Gross 

On 10/14/2014 11:30 AM, David Vrabel wrote:

On 14/10/14 10:00, Juergen Gross wrote:

In case a race was detected during allocation of a new p2m tree
element in alloc_p2m() the new allocated mid_mfn page is freed without
updating the pointer to the found value in the tree. This will result
in overwriting the just freed page with the mfn of the p2m leaf.


Can this race actually happen?  i.e., does this need tagging for stable?


Good question. I just stumbled over it while writing the linear p2m-list
patch.

Is it possible for gnttab_map_refs() to call set_foreign_p2m_mapping()
specifying a pfn which has been invalid before? In this case the race
could happen in dom0.

I think ballooning alone can't trigger this race, as it is calling
set_phys_to_machine() under lock only.

Juergen



David


--- a/arch/x86/xen/p2m.c
+++ b/arch/x86/xen/p2m.c
@@ -566,6 +566,7 @@ static bool alloc_p2m(unsigned long pfn)
/* Separately check the mid mfn level */
unsigned long missing_mfn;
unsigned long mid_mfn_mfn;
+   unsigned long old_mfn;

mid_mfn = alloc_p2m_page();
if (!mid_mfn)
@@ -575,10 +576,13 @@ static bool alloc_p2m(unsigned long pfn)

missing_mfn = virt_to_mfn(p2m_mid_missing_mfn);
mid_mfn_mfn = virt_to_mfn(mid_mfn);
-   if (cmpxchg(top_mfn_p, missing_mfn, mid_mfn_mfn) != missing_mfn)
+   old_mfn = cmpxchg(top_mfn_p, missing_mfn, mid_mfn_mfn);
+   if (old_mfn != missing_mfn) {
free_p2m_page(mid_mfn);
-   else
+   mid_mfn = mfn_to_virt(old_mfn);
+   } else {
p2m_top_mfn_p[topidx] = mid_mfn;
+   }
}

if (p2m_top[topidx][mididx] == p2m_identity ||



--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/



--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [Xen-devel] [PATCH] xen: avoid writing to freed memory after race in p2m handling

2014-10-14 Thread David Vrabel 
On 14/10/14 10:00, Juergen Gross wrote:
> In case a race was detected during allocation of a new p2m tree
> element in alloc_p2m() the new allocated mid_mfn page is freed without
> updating the pointer to the found value in the tree. This will result
> in overwriting the just freed page with the mfn of the p2m leaf.

Can this race actually happen?  i.e., does this need tagging for stable?

David

> --- a/arch/x86/xen/p2m.c
> +++ b/arch/x86/xen/p2m.c
> @@ -566,6 +566,7 @@ static bool alloc_p2m(unsigned long pfn)
>   /* Separately check the mid mfn level */
>   unsigned long missing_mfn;
>   unsigned long mid_mfn_mfn;
> + unsigned long old_mfn;
>  
>   mid_mfn = alloc_p2m_page();
>   if (!mid_mfn)
> @@ -575,10 +576,13 @@ static bool alloc_p2m(unsigned long pfn)
>  
>   missing_mfn = virt_to_mfn(p2m_mid_missing_mfn);
>   mid_mfn_mfn = virt_to_mfn(mid_mfn);
> - if (cmpxchg(top_mfn_p, missing_mfn, mid_mfn_mfn) != missing_mfn)
> + old_mfn = cmpxchg(top_mfn_p, missing_mfn, mid_mfn_mfn);
> + if (old_mfn != missing_mfn) {
>   free_p2m_page(mid_mfn);
> - else
> + mid_mfn = mfn_to_virt(old_mfn);
> + } else {
>   p2m_top_mfn_p[topidx] = mid_mfn;
> + }
>   }
>  
>   if (p2m_top[topidx][mididx] == p2m_identity ||
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH] xen: avoid writing to freed memory after race in p2m handling

2014-10-14 Thread Juergen Gross 
In case a race was detected during allocation of a new p2m tree
element in alloc_p2m() the new allocated mid_mfn page is freed without
updating the pointer to the found value in the tree. This will result
in overwriting the just freed page with the mfn of the p2m leaf.

Signed-off-by: Juergen Gross 
---
 arch/x86/xen/p2m.c | 8 ++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/arch/x86/xen/p2m.c b/arch/x86/xen/p2m.c
index 9f5983b..4534320 100644
--- a/arch/x86/xen/p2m.c
+++ b/arch/x86/xen/p2m.c
@@ -566,6 +566,7 @@ static bool alloc_p2m(unsigned long pfn)
/* Separately check the mid mfn level */
unsigned long missing_mfn;
unsigned long mid_mfn_mfn;
+   unsigned long old_mfn;
 
mid_mfn = alloc_p2m_page();
if (!mid_mfn)
@@ -575,10 +576,13 @@ static bool alloc_p2m(unsigned long pfn)
 
missing_mfn = virt_to_mfn(p2m_mid_missing_mfn);
mid_mfn_mfn = virt_to_mfn(mid_mfn);
-   if (cmpxchg(top_mfn_p, missing_mfn, mid_mfn_mfn) != missing_mfn)
+   old_mfn = cmpxchg(top_mfn_p, missing_mfn, mid_mfn_mfn);
+   if (old_mfn != missing_mfn) {
free_p2m_page(mid_mfn);
-   else
+   mid_mfn = mfn_to_virt(old_mfn);
+   } else {
p2m_top_mfn_p[topidx] = mid_mfn;
+   }
}
 
if (p2m_top[topidx][mididx] == p2m_identity ||
-- 
1.8.4.5

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH] xen: avoid writing to freed memory after race in p2m handling

2014-10-14 Thread Juergen Gross 
In case a race was detected during allocation of a new p2m tree
element in alloc_p2m() the new allocated mid_mfn page is freed without
updating the pointer to the found value in the tree. This will result
in overwriting the just freed page with the mfn of the p2m leaf.

Signed-off-by: Juergen Gross jgr...@suse.com
---
 arch/x86/xen/p2m.c | 8 ++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/arch/x86/xen/p2m.c b/arch/x86/xen/p2m.c
index 9f5983b..4534320 100644
--- a/arch/x86/xen/p2m.c
+++ b/arch/x86/xen/p2m.c
@@ -566,6 +566,7 @@ static bool alloc_p2m(unsigned long pfn)
/* Separately check the mid mfn level */
unsigned long missing_mfn;
unsigned long mid_mfn_mfn;
+   unsigned long old_mfn;
 
mid_mfn = alloc_p2m_page();
if (!mid_mfn)
@@ -575,10 +576,13 @@ static bool alloc_p2m(unsigned long pfn)
 
missing_mfn = virt_to_mfn(p2m_mid_missing_mfn);
mid_mfn_mfn = virt_to_mfn(mid_mfn);
-   if (cmpxchg(top_mfn_p, missing_mfn, mid_mfn_mfn) != missing_mfn)
+   old_mfn = cmpxchg(top_mfn_p, missing_mfn, mid_mfn_mfn);
+   if (old_mfn != missing_mfn) {
free_p2m_page(mid_mfn);
-   else
+   mid_mfn = mfn_to_virt(old_mfn);
+   } else {
p2m_top_mfn_p[topidx] = mid_mfn;
+   }
}
 
if (p2m_top[topidx][mididx] == p2m_identity ||
-- 
1.8.4.5

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [Xen-devel] [PATCH] xen: avoid writing to freed memory after race in p2m handling

2014-10-14 Thread David Vrabel 
On 14/10/14 10:00, Juergen Gross wrote:
 In case a race was detected during allocation of a new p2m tree
 element in alloc_p2m() the new allocated mid_mfn page is freed without
 updating the pointer to the found value in the tree. This will result
 in overwriting the just freed page with the mfn of the p2m leaf.

Can this race actually happen?  i.e., does this need tagging for stable?

David

 --- a/arch/x86/xen/p2m.c
 +++ b/arch/x86/xen/p2m.c
 @@ -566,6 +566,7 @@ static bool alloc_p2m(unsigned long pfn)
   /* Separately check the mid mfn level */
   unsigned long missing_mfn;
   unsigned long mid_mfn_mfn;
 + unsigned long old_mfn;
  
   mid_mfn = alloc_p2m_page();
   if (!mid_mfn)
 @@ -575,10 +576,13 @@ static bool alloc_p2m(unsigned long pfn)
  
   missing_mfn = virt_to_mfn(p2m_mid_missing_mfn);
   mid_mfn_mfn = virt_to_mfn(mid_mfn);
 - if (cmpxchg(top_mfn_p, missing_mfn, mid_mfn_mfn) != missing_mfn)
 + old_mfn = cmpxchg(top_mfn_p, missing_mfn, mid_mfn_mfn);
 + if (old_mfn != missing_mfn) {
   free_p2m_page(mid_mfn);
 - else
 + mid_mfn = mfn_to_virt(old_mfn);
 + } else {
   p2m_top_mfn_p[topidx] = mid_mfn;
 + }
   }
  
   if (p2m_top[topidx][mididx] == p2m_identity ||
 

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [Xen-devel] [PATCH] xen: avoid writing to freed memory after race in p2m handling

2014-10-14 Thread Juergen Gross 

On 10/14/2014 11:30 AM, David Vrabel wrote:

On 14/10/14 10:00, Juergen Gross wrote:

In case a race was detected during allocation of a new p2m tree
element in alloc_p2m() the new allocated mid_mfn page is freed without
updating the pointer to the found value in the tree. This will result
in overwriting the just freed page with the mfn of the p2m leaf.


Can this race actually happen?  i.e., does this need tagging for stable?


Good question. I just stumbled over it while writing the linear p2m-list
patch.

Is it possible for gnttab_map_refs() to call set_foreign_p2m_mapping()
specifying a pfn which has been invalid before? In this case the race
could happen in dom0.

I think ballooning alone can't trigger this race, as it is calling
set_phys_to_machine() under lock only.

Juergen



David


--- a/arch/x86/xen/p2m.c
+++ b/arch/x86/xen/p2m.c
@@ -566,6 +566,7 @@ static bool alloc_p2m(unsigned long pfn)
/* Separately check the mid mfn level */
unsigned long missing_mfn;
unsigned long mid_mfn_mfn;
+   unsigned long old_mfn;

mid_mfn = alloc_p2m_page();
if (!mid_mfn)
@@ -575,10 +576,13 @@ static bool alloc_p2m(unsigned long pfn)

missing_mfn = virt_to_mfn(p2m_mid_missing_mfn);
mid_mfn_mfn = virt_to_mfn(mid_mfn);
-   if (cmpxchg(top_mfn_p, missing_mfn, mid_mfn_mfn) != missing_mfn)
+   old_mfn = cmpxchg(top_mfn_p, missing_mfn, mid_mfn_mfn);
+   if (old_mfn != missing_mfn) {
free_p2m_page(mid_mfn);
-   else
+   mid_mfn = mfn_to_virt(old_mfn);
+   } else {
p2m_top_mfn_p[topidx] = mid_mfn;
+   }
}

if (p2m_top[topidx][mididx] == p2m_identity ||



--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/



--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [Xen-devel] [PATCH] xen: avoid writing to freed memory after race in p2m handling

2014-10-14 Thread David Vrabel 
On 14/10/14 10:49, Juergen Gross wrote:
 On 10/14/2014 11:30 AM, David Vrabel wrote:
 On 14/10/14 10:00, Juergen Gross wrote:
 In case a race was detected during allocation of a new p2m tree
 element in alloc_p2m() the new allocated mid_mfn page is freed without
 updating the pointer to the found value in the tree. This will result
 in overwriting the just freed page with the mfn of the p2m leaf.

 Can this race actually happen?  i.e., does this need tagging for stable?
 
 Good question. I just stumbled over it while writing the linear p2m-list
 patch.
 
 Is it possible for gnttab_map_refs() to call set_foreign_p2m_mapping()
 specifying a pfn which has been invalid before? In this case the race
 could happen in dom0.

Yes, if two backends map into ballooned pages from a region of
untouched,  pre-ballooned memory.  But these seems super rare and I
don't think there have been any bug reports that could be attributed to
this, so I don't think a stable backport is needed.

 I think ballooning alone can't trigger this race, as it is calling
 set_phys_to_machine() under lock only.

Agreed.

Applied to stable/for-linus-3.18.

Thanks.

David
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/