[PATCH] NPT: temporarily retain page table mapping in do_recalc()

[PATCH] NPT: temporarily retain page table mapping in do_recalc()

[Jan Beulich]

[-- Attachment #1: Type: text/plain, Size: 3462 bytes --]

Commit b3e024f3 ("x86/NPT: don't walk page tables when changing types
on a range") neglected the fact that p2m_next_level() replaces the
previous level's mapping with the new level's one, hence dereferencing
a stale pointer the translation for which may no longer be available
(timing dependent). Add a parameter to that function allowing the
caller to request that the mapping be retained (the unmapping will be
taken care of by the caller then).

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/arch/x86/mm/p2m-pt.c
+++ b/xen/arch/x86/mm/p2m-pt.c
@@ -172,7 +172,7 @@ static void p2m_add_iommu_flags(l1_pgent
 static int
 p2m_next_level(struct p2m_domain *p2m, void **table,
                unsigned long *gfn_remainder, unsigned long gfn, u32 shift,
-               u32 max, unsigned long type)
+               u32 max, unsigned long type, bool_t unmap)
 {
     l1_pgentry_t *l1_entry;
     l1_pgentry_t *p2m_entry;
@@ -280,7 +280,8 @@ p2m_next_level(struct p2m_domain *p2m, v
     }
 
     next = map_domain_page(l1e_get_pfn(*p2m_entry));
-    unmap_domain_page(*table);
+    if ( unmap )
+        unmap_domain_page(*table);
     *table = next;
 
     return 0;
@@ -319,7 +320,7 @@ static int p2m_pt_set_recalc_range(struc
 
         err = p2m_next_level(p2m, &table, &gfn_remainder, first_gfn,
                              i * PAGETABLE_ORDER, 1 << PAGETABLE_ORDER,
-                             pgt[i - 1]);
+                             pgt[i - 1], 1);
         if ( err )
             goto out;
     }
@@ -386,7 +387,7 @@ static int do_recalc(struct p2m_domain *
 
         err = p2m_next_level(p2m, &table, &gfn_remainder, gfn,
                              level * PAGETABLE_ORDER, 1 << PAGETABLE_ORDER,
-                             pgt[level - 1]);
+                             pgt[level - 1], 0);
         if ( err )
             goto out;
 
@@ -416,6 +417,7 @@ static int do_recalc(struct p2m_domain *
             clear_recalc(l1, e);
             p2m->write_p2m_entry(p2m, gfn, pent, e, level + 1);
         }
+        unmap_domain_page((void *)((unsigned long)pent & PAGE_MASK));
     }
 
     pent = p2m_find_entry(table, &gfn_remainder, gfn,
@@ -519,7 +521,7 @@ p2m_pt_set_entry(struct p2m_domain *p2m,
     table = map_domain_page(mfn_x(pagetable_get_mfn(p2m_get_pagetable(p2m))));
     rc = p2m_next_level(p2m, &table, &gfn_remainder, gfn,
                         L4_PAGETABLE_SHIFT - PAGE_SHIFT,
-                        L4_PAGETABLE_ENTRIES, PGT_l3_page_table);
+                        L4_PAGETABLE_ENTRIES, PGT_l3_page_table, 1);
     if ( rc )
         goto out;
 
@@ -565,7 +567,7 @@ p2m_pt_set_entry(struct p2m_domain *p2m,
     {
         rc = p2m_next_level(p2m, &table, &gfn_remainder, gfn,
                             L3_PAGETABLE_SHIFT - PAGE_SHIFT,
-                            L3_PAGETABLE_ENTRIES, PGT_l2_page_table);
+                            L3_PAGETABLE_ENTRIES, PGT_l2_page_table, 1);
         if ( rc )
             goto out;
     }
@@ -574,7 +576,7 @@ p2m_pt_set_entry(struct p2m_domain *p2m,
     {
         rc = p2m_next_level(p2m, &table, &gfn_remainder, gfn,
                             L2_PAGETABLE_SHIFT - PAGE_SHIFT,
-                            L2_PAGETABLE_ENTRIES, PGT_l1_page_table);
+                            L2_PAGETABLE_ENTRIES, PGT_l1_page_table, 1);
         if ( rc )
             goto out;
 




[-- Attachment #2: NPT-recalc-keep-mapping.patch --]
[-- Type: text/plain, Size: 3517 bytes --]

NPT: temporarily retain page table mapping in do_recalc()

Commit b3e024f3 ("x86/NPT: don't walk page tables when changing types
on a range") neglected the fact that p2m_next_level() replaces the
previous level's mapping with the new level's one, hence dereferencing
a stale pointer the translation for which may no longer be available
(timing dependent). Add a parameter to that function allowing the
caller to request that the mapping be retained (the unmapping will be
taken care of by the caller then).

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/arch/x86/mm/p2m-pt.c
+++ b/xen/arch/x86/mm/p2m-pt.c
@@ -172,7 +172,7 @@ static void p2m_add_iommu_flags(l1_pgent
 static int
 p2m_next_level(struct p2m_domain *p2m, void **table,
                unsigned long *gfn_remainder, unsigned long gfn, u32 shift,
-               u32 max, unsigned long type)
+               u32 max, unsigned long type, bool_t unmap)
 {
     l1_pgentry_t *l1_entry;
     l1_pgentry_t *p2m_entry;
@@ -280,7 +280,8 @@ p2m_next_level(struct p2m_domain *p2m, v
     }
 
     next = map_domain_page(l1e_get_pfn(*p2m_entry));
-    unmap_domain_page(*table);
+    if ( unmap )
+        unmap_domain_page(*table);
     *table = next;
 
     return 0;
@@ -319,7 +320,7 @@ static int p2m_pt_set_recalc_range(struc
 
         err = p2m_next_level(p2m, &table, &gfn_remainder, first_gfn,
                              i * PAGETABLE_ORDER, 1 << PAGETABLE_ORDER,
-                             pgt[i - 1]);
+                             pgt[i - 1], 1);
         if ( err )
             goto out;
     }
@@ -386,7 +387,7 @@ static int do_recalc(struct p2m_domain *
 
         err = p2m_next_level(p2m, &table, &gfn_remainder, gfn,
                              level * PAGETABLE_ORDER, 1 << PAGETABLE_ORDER,
-                             pgt[level - 1]);
+                             pgt[level - 1], 0);
         if ( err )
             goto out;
 
@@ -416,6 +417,7 @@ static int do_recalc(struct p2m_domain *
             clear_recalc(l1, e);
             p2m->write_p2m_entry(p2m, gfn, pent, e, level + 1);
         }
+        unmap_domain_page((void *)((unsigned long)pent & PAGE_MASK));
     }
 
     pent = p2m_find_entry(table, &gfn_remainder, gfn,
@@ -519,7 +521,7 @@ p2m_pt_set_entry(struct p2m_domain *p2m,
     table = map_domain_page(mfn_x(pagetable_get_mfn(p2m_get_pagetable(p2m))));
     rc = p2m_next_level(p2m, &table, &gfn_remainder, gfn,
                         L4_PAGETABLE_SHIFT - PAGE_SHIFT,
-                        L4_PAGETABLE_ENTRIES, PGT_l3_page_table);
+                        L4_PAGETABLE_ENTRIES, PGT_l3_page_table, 1);
     if ( rc )
         goto out;
 
@@ -565,7 +567,7 @@ p2m_pt_set_entry(struct p2m_domain *p2m,
     {
         rc = p2m_next_level(p2m, &table, &gfn_remainder, gfn,
                             L3_PAGETABLE_SHIFT - PAGE_SHIFT,
-                            L3_PAGETABLE_ENTRIES, PGT_l2_page_table);
+                            L3_PAGETABLE_ENTRIES, PGT_l2_page_table, 1);
         if ( rc )
             goto out;
     }
@@ -574,7 +576,7 @@ p2m_pt_set_entry(struct p2m_domain *p2m,
     {
         rc = p2m_next_level(p2m, &table, &gfn_remainder, gfn,
                             L2_PAGETABLE_SHIFT - PAGE_SHIFT,
-                            L2_PAGETABLE_ENTRIES, PGT_l1_page_table);
+                            L2_PAGETABLE_ENTRIES, PGT_l1_page_table, 1);
         if ( rc )
             goto out;
 

[-- Attachment #3: Type: text/plain, Size: 126 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH] NPT: temporarily retain page table mapping in do_recalc()

[Ian Campbell]

On Mon, 2014-05-05 at 11:31 +0100, Jan Beulich wrote:
> Commit b3e024f3 ("x86/NPT: don't walk page tables when changing types
> on a range") neglected the fact that p2m_next_level() replaces the
> previous level's mapping with the new level's one, hence dereferencing
> a stale pointer the translation for which may no longer be available
> (timing dependent). Add a parameter to that function allowing the
> caller to request that the mapping be retained (the unmapping will be
> taken care of by the caller then).
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

FWIW: Reviewed-by: Ian Campbell <ian.campbell@citrix.com>

Ian.

Re: [PATCH] NPT: temporarily retain page table mapping in do_recalc()

[Andrew Cooper]

[-- Attachment #1.1: Type: text/plain, Size: 4194 bytes --]

On 05/05/14 11:31, Jan Beulich wrote:
> Commit b3e024f3 ("x86/NPT: don't walk page tables when changing types
> on a range") neglected the fact that p2m_next_level() replaces the
> previous level's mapping with the new level's one, hence dereferencing
> a stale pointer the translation for which may no longer be available
> (timing dependent). Add a parameter to that function allowing the
> caller to request that the mapping be retained (the unmapping will be
> taken care of by the caller then).
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Given the new API for p2m_next_level(), it is important to note that the
mapping can only be safely left in place on the success path.  In the
case of an error the page must be unmapped even if 'unmap' is false.

While the current behaviour is safe, all it would take is an innocent
refactoring of the error paths to invalidate this requirement.

Is it worth leaving a comment to that effect?

Functionally however, Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

>
> --- a/xen/arch/x86/mm/p2m-pt.c
> +++ b/xen/arch/x86/mm/p2m-pt.c
> @@ -172,7 +172,7 @@ static void p2m_add_iommu_flags(l1_pgent
>  static int
>  p2m_next_level(struct p2m_domain *p2m, void **table,
>                 unsigned long *gfn_remainder, unsigned long gfn, u32 shift,
> -               u32 max, unsigned long type)
> +               u32 max, unsigned long type, bool_t unmap)
>  {
>      l1_pgentry_t *l1_entry;
>      l1_pgentry_t *p2m_entry;
> @@ -280,7 +280,8 @@ p2m_next_level(struct p2m_domain *p2m, v
>      }
>  
>      next = map_domain_page(l1e_get_pfn(*p2m_entry));
> -    unmap_domain_page(*table);
> +    if ( unmap )
> +        unmap_domain_page(*table);
>      *table = next;
>  
>      return 0;
> @@ -319,7 +320,7 @@ static int p2m_pt_set_recalc_range(struc
>  
>          err = p2m_next_level(p2m, &table, &gfn_remainder, first_gfn,
>                               i * PAGETABLE_ORDER, 1 << PAGETABLE_ORDER,
> -                             pgt[i - 1]);
> +                             pgt[i - 1], 1);
>          if ( err )
>              goto out;
>      }
> @@ -386,7 +387,7 @@ static int do_recalc(struct p2m_domain *
>  
>          err = p2m_next_level(p2m, &table, &gfn_remainder, gfn,
>                               level * PAGETABLE_ORDER, 1 << PAGETABLE_ORDER,
> -                             pgt[level - 1]);
> +                             pgt[level - 1], 0);
>          if ( err )
>              goto out;
>  
> @@ -416,6 +417,7 @@ static int do_recalc(struct p2m_domain *
>              clear_recalc(l1, e);
>              p2m->write_p2m_entry(p2m, gfn, pent, e, level + 1);
>          }
> +        unmap_domain_page((void *)((unsigned long)pent & PAGE_MASK));
>      }
>  
>      pent = p2m_find_entry(table, &gfn_remainder, gfn,
> @@ -519,7 +521,7 @@ p2m_pt_set_entry(struct p2m_domain *p2m,
>      table = map_domain_page(mfn_x(pagetable_get_mfn(p2m_get_pagetable(p2m))));
>      rc = p2m_next_level(p2m, &table, &gfn_remainder, gfn,
>                          L4_PAGETABLE_SHIFT - PAGE_SHIFT,
> -                        L4_PAGETABLE_ENTRIES, PGT_l3_page_table);
> +                        L4_PAGETABLE_ENTRIES, PGT_l3_page_table, 1);
>      if ( rc )
>          goto out;
>  
> @@ -565,7 +567,7 @@ p2m_pt_set_entry(struct p2m_domain *p2m,
>      {
>          rc = p2m_next_level(p2m, &table, &gfn_remainder, gfn,
>                              L3_PAGETABLE_SHIFT - PAGE_SHIFT,
> -                            L3_PAGETABLE_ENTRIES, PGT_l2_page_table);
> +                            L3_PAGETABLE_ENTRIES, PGT_l2_page_table, 1);
>          if ( rc )
>              goto out;
>      }
> @@ -574,7 +576,7 @@ p2m_pt_set_entry(struct p2m_domain *p2m,
>      {
>          rc = p2m_next_level(p2m, &table, &gfn_remainder, gfn,
>                              L2_PAGETABLE_SHIFT - PAGE_SHIFT,
> -                            L2_PAGETABLE_ENTRIES, PGT_l1_page_table);
> +                            L2_PAGETABLE_ENTRIES, PGT_l1_page_table, 1);
>          if ( rc )
>              goto out;
>  
>
>
>
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel


[-- Attachment #1.2: Type: text/html, Size: 5086 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH] NPT: temporarily retain page table mapping in do_recalc()

[Jan Beulich]

>>> On 06.05.14 at 11:54, <andrew.cooper3@citrix.com> wrote:
> On 05/05/14 11:31, Jan Beulich wrote:
>> Commit b3e024f3 ("x86/NPT: don't walk page tables when changing types
>> on a range") neglected the fact that p2m_next_level() replaces the
>> previous level's mapping with the new level's one, hence dereferencing
>> a stale pointer the translation for which may no longer be available
>> (timing dependent). Add a parameter to that function allowing the
>> caller to request that the mapping be retained (the unmapping will be
>> taken care of by the caller then).
>>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> Given the new API for p2m_next_level(), it is important to note that the
> mapping can only be safely left in place on the success path.  In the
> case of an error the page must be unmapped even if 'unmap' is false.
> 
> While the current behaviour is safe, all it would take is an innocent
> refactoring of the error paths to invalidate this requirement.

But that's not a correct description of how this behaves: The
function _always_ leaves a mapping in place. The new parameter
is used to tell it to return with _two_ established mappings - the
one that was there on entry and the one needed for the next
level. Consequently no error path should ever reach this map/
unmap pair near the end of the function.

Jan

Re: [PATCH] NPT: temporarily retain page table mapping in do_recalc()

[Ian Campbell]

On Tue, 2014-05-06 at 11:02 +0100, Jan Beulich wrote:
> >>> On 06.05.14 at 11:54, <andrew.cooper3@citrix.com> wrote:
> > On 05/05/14 11:31, Jan Beulich wrote:
> >> Commit b3e024f3 ("x86/NPT: don't walk page tables when changing types
> >> on a range") neglected the fact that p2m_next_level() replaces the
> >> previous level's mapping with the new level's one, hence dereferencing
> >> a stale pointer the translation for which may no longer be available
> >> (timing dependent). Add a parameter to that function allowing the
> >> caller to request that the mapping be retained (the unmapping will be
> >> taken care of by the caller then).
> >>
> >> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> > 
> > Given the new API for p2m_next_level(), it is important to note that the
> > mapping can only be safely left in place on the success path.  In the
> > case of an error the page must be unmapped even if 'unmap' is false.
> > 
> > While the current behaviour is safe, all it would take is an innocent
> > refactoring of the error paths to invalidate this requirement.
> 
> But that's not a correct description of how this behaves: The
> function _always_ leaves a mapping in place. The new parameter
> is used to tell it to return with _two_ established mappings - the
> one that was there on entry and the one needed for the next
> level. Consequently no error path should ever reach this map/
> unmap pair near the end of the function.
> 

That was my conclusion when reviewing too. It took a bit of time to
convince myself it was true, but it was.

Ian.

Re: [PATCH] NPT: temporarily retain page table mapping in do_recalc()

[Tim Deegan]

At 11:31 +0100 on 05 May (1399285915), Jan Beulich wrote:
> Commit b3e024f3 ("x86/NPT: don't walk page tables when changing types
> on a range") neglected the fact that p2m_next_level() replaces the
> previous level's mapping with the new level's one, hence dereferencing
> a stale pointer the translation for which may no longer be available
> (timing dependent). Add a parameter to that function allowing the
> caller to request that the mapping be retained (the unmapping will be
> taken care of by the caller then).
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

That's not a very nice interface, but I can't see a way around it,
since do_recalc() needs to have the lower-level mapping available before
it writes the upper-level entry, and having all callers of
p2m_next_level() do their own unmap would be awful too.

Reviewed-by: Tim Deegan <tim@xen.org>