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 <[email protected]>

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 <[email protected]>

xen/arch/x86/mm/p2m-pt.c +9 -7

@@ -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
[email protected]
http://lists.xen.org/xen-devel

Quoted 1 line

Quoted 1 line · On 06.05.14 at 11:54, <[email protected]> wrote:

Quoted 8 lines · Jan Beulich

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

Quoted 1 line

Signed-off-by Jan Beulich <[email protected]>

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.

Quoted 10 lines

Quoted 1 line

Quoted 1 line · On 06.05.14 at 11:54, <[email protected]> wrote:

Quoted 8 lines · Jan Beulich

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

Quoted 8 lines

Quoted 1 line

Signed-off-by Jan Beulich <[email protected]>

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.

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 <[email protected]>