Patchwork [v3,1/3] x86/p2m-pt: simplify p2m_next_level()

login
register
mail settings
Submitter Jan Beulich
Date Aug. 11, 2017, 1:19 p.m.
Message ID <598DCB00020000780016EE0D@prv-mh.provo.novell.com>
Download mbox | patch
Permalink /patch/314245/
State New
Headers show

Comments

Jan Beulich - Aug. 11, 2017, 1:19 p.m.
Calculate entry PFN and flags just once. Convert the two successive
main if()-s to and if/else-if chain. Restrict variable scope where
reasonable. Take the opportunity and also make the induction variable
unsigned.

This at once fixes excessive permissions granted in the 2M PTEs
resulting from splitting a 1G one - original permissions should be
inherited instead. This is not a security issue only because all of
this takes no effect anyway, as iommu_hap_pt_share is always false on
AMD systems for all supported branches.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
v3: Fix IOMMU permission handling for shattered PTEs.
v2: Re-do mostly from scratch following review feedback.
Jan Beulich - Aug. 29, 2017, 8:40 a.m.
>>> On 11.08.17 at 15:19, <JBeulich@suse.com> wrote:
> Calculate entry PFN and flags just once. Convert the two successive
> main if()-s to and if/else-if chain. Restrict variable scope where
> reasonable. Take the opportunity and also make the induction variable
> unsigned.
> 
> This at once fixes excessive permissions granted in the 2M PTEs
> resulting from splitting a 1G one - original permissions should be
> inherited instead. This is not a security issue only because all of
> this takes no effect anyway, as iommu_hap_pt_share is always false on
> AMD systems for all supported branches.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> ---
> v3: Fix IOMMU permission handling for shattered PTEs.
> v2: Re-do mostly from scratch following review feedback.
> 
> --- a/xen/arch/x86/mm/p2m-pt.c
> +++ b/xen/arch/x86/mm/p2m-pt.c
> @@ -191,18 +191,18 @@ p2m_next_level(struct p2m_domain *p2m, v
>                 unsigned long *gfn_remainder, unsigned long gfn, u32 shift,
>                 u32 max, unsigned long type, bool_t unmap)
>  {
> -    l1_pgentry_t *l1_entry;
> -    l1_pgentry_t *p2m_entry;
> -    l1_pgentry_t new_entry;
> +    l1_pgentry_t *p2m_entry, new_entry;
>      void *next;
> -    int i;
> +    unsigned int flags;
>  
>      if ( !(p2m_entry = p2m_find_entry(*table, gfn_remainder, gfn,
>                                        shift, max)) )
>          return -ENOENT;
>  
> +    flags = l1e_get_flags(*p2m_entry);
> +
>      /* PoD/paging: Not present doesn't imply empty. */
> -    if ( !l1e_get_flags(*p2m_entry) )
> +    if ( !flags )
>      {
>          struct page_info *pg;
>  
> @@ -231,70 +231,67 @@ p2m_next_level(struct p2m_domain *p2m, v
>              break;
>          }
>      }
> -
> -    ASSERT(l1e_get_flags(*p2m_entry) & (_PAGE_PRESENT|_PAGE_PSE));
> -
> -    /* split 1GB pages into 2MB pages */
> -    if ( type == PGT_l2_page_table && (l1e_get_flags(*p2m_entry) & _PAGE_PSE) 
> )
> +    else if ( flags & _PAGE_PSE )
>      {
> -        unsigned long flags, pfn;
> +        /* Split superpages pages into smaller ones. */
> +        unsigned long pfn = l1e_get_pfn(*p2m_entry);
>          struct page_info *pg;
> +        l1_pgentry_t *l1_entry;
> +        unsigned int i, level;
>  
> -        pg = p2m_alloc_ptp(p2m, PGT_l2_page_table);
> -        if ( pg == NULL )
> -            return -ENOMEM;
> -
> -        flags = l1e_get_flags(*p2m_entry);
> -        pfn = l1e_get_pfn(*p2m_entry);
> -
> -        l1_entry = __map_domain_page(pg);
> -        for ( i = 0; i < L2_PAGETABLE_ENTRIES; i++ )
> +        switch ( type )
>          {
> -            new_entry = l1e_from_pfn(pfn | (i * L1_PAGETABLE_ENTRIES), 
> flags);
> -            p2m_add_iommu_flags(&new_entry, 1, 
> IOMMUF_readable|IOMMUF_writable);
> -            p2m->write_p2m_entry(p2m, gfn, l1_entry + i, new_entry, 2);
> -        }
> -        unmap_domain_page(l1_entry);
> -        new_entry = l1e_from_pfn(mfn_x(page_to_mfn(pg)),
> -                                 P2M_BASE_FLAGS | _PAGE_RW); /* disable PSE 
> */
> -        p2m_add_iommu_flags(&new_entry, 2, IOMMUF_readable|IOMMUF_writable);
> -        p2m->write_p2m_entry(p2m, gfn, p2m_entry, new_entry, 3);
> -    }
> +        case PGT_l2_page_table:
> +            level = 2;
> +            break;
>  
> +        case PGT_l1_page_table:
> +            /*
> +             * New splintered mappings inherit the flags of the old 
> superpage,
> +             * with a little reorganisation for the _PAGE_PSE_PAT bit.
> +             */
> +            if ( pfn & 1 )           /* ==> _PAGE_PSE_PAT was set */
> +                pfn -= 1;            /* Clear it; _PAGE_PSE becomes 
> _PAGE_PAT */
> +            else
> +                flags &= ~_PAGE_PSE; /* Clear _PAGE_PSE (== _PAGE_PAT) */
>  
> -    /* split single 2MB large page into 4KB page in P2M table */
> -    if ( type == PGT_l1_page_table && (l1e_get_flags(*p2m_entry) & _PAGE_PSE) 
> )
> -    {
> -        unsigned long flags, pfn;
> -        struct page_info *pg;
> +            level = 1;
> +            break;
> +
> +        default:
> +            ASSERT_UNREACHABLE();
> +            return -EINVAL;
> +        }
>  
> -        pg = p2m_alloc_ptp(p2m, PGT_l1_page_table);
> +        pg = p2m_alloc_ptp(p2m, type);
>          if ( pg == NULL )
>              return -ENOMEM;
>  
> -        /* New splintered mappings inherit the flags of the old superpage, 
> -         * with a little reorganisation for the _PAGE_PSE_PAT bit. */
> -        flags = l1e_get_flags(*p2m_entry);
> -        pfn = l1e_get_pfn(*p2m_entry);
> -        if ( pfn & 1 )           /* ==> _PAGE_PSE_PAT was set */
> -            pfn -= 1;            /* Clear it; _PAGE_PSE becomes _PAGE_PAT 
> */
> -        else
> -            flags &= ~_PAGE_PSE; /* Clear _PAGE_PSE (== _PAGE_PAT) */
> -        
>          l1_entry = __map_domain_page(pg);
> -        for ( i = 0; i < L1_PAGETABLE_ENTRIES; i++ )
> +
> +        /* Inherit original IOMMU permissions, but update Next Level. */
> +        if ( iommu_hap_pt_share )
>          {
> -            new_entry = l1e_from_pfn(pfn | i, flags);
> -            p2m_add_iommu_flags(&new_entry, 0, 0);
> -            p2m->write_p2m_entry(p2m, gfn, l1_entry + i, new_entry, 1);
> +            flags &= ~iommu_nlevel_to_flags(~0, 0);
> +            flags |= iommu_nlevel_to_flags(level - 1, 0);
>          }
> +
> +        for ( i = 0; i < (1u << PAGETABLE_ORDER); i++ )
> +        {
> +            new_entry = l1e_from_pfn(pfn | (i << ((level - 1) * 
> PAGETABLE_ORDER)),
> +                                     flags);
> +            p2m->write_p2m_entry(p2m, gfn, l1_entry + i, new_entry, level);
> +        }
> +
>          unmap_domain_page(l1_entry);
> -        
> +
>          new_entry = l1e_from_pfn(mfn_x(page_to_mfn(pg)),
>                                   P2M_BASE_FLAGS | _PAGE_RW);
> -        p2m_add_iommu_flags(&new_entry, 1, IOMMUF_readable|IOMMUF_writable);
> -        p2m->write_p2m_entry(p2m, gfn, p2m_entry, new_entry, 2);
> +        p2m_add_iommu_flags(&new_entry, level, 
> IOMMUF_readable|IOMMUF_writable);
> +        p2m->write_p2m_entry(p2m, gfn, p2m_entry, new_entry, level + 1);
>      }
> +    else
> +        ASSERT(flags & _PAGE_PRESENT);
>  
>      next = map_domain_page(_mfn(l1e_get_pfn(*p2m_entry)));
>      if ( unmap )
> 
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org 
> https://lists.xen.org/xen-devel
George Dunlap - Sept. 4, 2017, 1:28 p.m.
On 08/11/2017 02:19 PM, Jan Beulich wrote:
> Calculate entry PFN and flags just once. Convert the two successive
> main if()-s to and if/else-if chain. Restrict variable scope where
> reasonable. Take the opportunity and also make the induction variable
> unsigned.
> 
> This at once fixes excessive permissions granted in the 2M PTEs
> resulting from splitting a 1G one - original permissions should be
> inherited instead. This is not a security issue only because all of
> this takes no effect anyway, as iommu_hap_pt_share is always false on
> AMD systems for all supported branches.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Acked-by: George Dunlap <george.dunlap@citrix.com>

Sorry for the delay.

Patch

--- a/xen/arch/x86/mm/p2m-pt.c
+++ b/xen/arch/x86/mm/p2m-pt.c
@@ -191,18 +191,18 @@  p2m_next_level(struct p2m_domain *p2m, v
                unsigned long *gfn_remainder, unsigned long gfn, u32 shift,
                u32 max, unsigned long type, bool_t unmap)
 {
-    l1_pgentry_t *l1_entry;
-    l1_pgentry_t *p2m_entry;
-    l1_pgentry_t new_entry;
+    l1_pgentry_t *p2m_entry, new_entry;
     void *next;
-    int i;
+    unsigned int flags;
 
     if ( !(p2m_entry = p2m_find_entry(*table, gfn_remainder, gfn,
                                       shift, max)) )
         return -ENOENT;
 
+    flags = l1e_get_flags(*p2m_entry);
+
     /* PoD/paging: Not present doesn't imply empty. */
-    if ( !l1e_get_flags(*p2m_entry) )
+    if ( !flags )
     {
         struct page_info *pg;
 
@@ -231,70 +231,67 @@  p2m_next_level(struct p2m_domain *p2m, v
             break;
         }
     }
-
-    ASSERT(l1e_get_flags(*p2m_entry) & (_PAGE_PRESENT|_PAGE_PSE));
-
-    /* split 1GB pages into 2MB pages */
-    if ( type == PGT_l2_page_table && (l1e_get_flags(*p2m_entry) & _PAGE_PSE) )
+    else if ( flags & _PAGE_PSE )
     {
-        unsigned long flags, pfn;
+        /* Split superpages pages into smaller ones. */
+        unsigned long pfn = l1e_get_pfn(*p2m_entry);
         struct page_info *pg;
+        l1_pgentry_t *l1_entry;
+        unsigned int i, level;
 
-        pg = p2m_alloc_ptp(p2m, PGT_l2_page_table);
-        if ( pg == NULL )
-            return -ENOMEM;
-
-        flags = l1e_get_flags(*p2m_entry);
-        pfn = l1e_get_pfn(*p2m_entry);
-
-        l1_entry = __map_domain_page(pg);
-        for ( i = 0; i < L2_PAGETABLE_ENTRIES; i++ )
+        switch ( type )
         {
-            new_entry = l1e_from_pfn(pfn | (i * L1_PAGETABLE_ENTRIES), flags);
-            p2m_add_iommu_flags(&new_entry, 1, IOMMUF_readable|IOMMUF_writable);
-            p2m->write_p2m_entry(p2m, gfn, l1_entry + i, new_entry, 2);
-        }
-        unmap_domain_page(l1_entry);
-        new_entry = l1e_from_pfn(mfn_x(page_to_mfn(pg)),
-                                 P2M_BASE_FLAGS | _PAGE_RW); /* disable PSE */
-        p2m_add_iommu_flags(&new_entry, 2, IOMMUF_readable|IOMMUF_writable);
-        p2m->write_p2m_entry(p2m, gfn, p2m_entry, new_entry, 3);
-    }
+        case PGT_l2_page_table:
+            level = 2;
+            break;
 
+        case PGT_l1_page_table:
+            /*
+             * New splintered mappings inherit the flags of the old superpage,
+             * with a little reorganisation for the _PAGE_PSE_PAT bit.
+             */
+            if ( pfn & 1 )           /* ==> _PAGE_PSE_PAT was set */
+                pfn -= 1;            /* Clear it; _PAGE_PSE becomes _PAGE_PAT */
+            else
+                flags &= ~_PAGE_PSE; /* Clear _PAGE_PSE (== _PAGE_PAT) */
 
-    /* split single 2MB large page into 4KB page in P2M table */
-    if ( type == PGT_l1_page_table && (l1e_get_flags(*p2m_entry) & _PAGE_PSE) )
-    {
-        unsigned long flags, pfn;
-        struct page_info *pg;
+            level = 1;
+            break;
+
+        default:
+            ASSERT_UNREACHABLE();
+            return -EINVAL;
+        }
 
-        pg = p2m_alloc_ptp(p2m, PGT_l1_page_table);
+        pg = p2m_alloc_ptp(p2m, type);
         if ( pg == NULL )
             return -ENOMEM;
 
-        /* New splintered mappings inherit the flags of the old superpage, 
-         * with a little reorganisation for the _PAGE_PSE_PAT bit. */
-        flags = l1e_get_flags(*p2m_entry);
-        pfn = l1e_get_pfn(*p2m_entry);
-        if ( pfn & 1 )           /* ==> _PAGE_PSE_PAT was set */
-            pfn -= 1;            /* Clear it; _PAGE_PSE becomes _PAGE_PAT */
-        else
-            flags &= ~_PAGE_PSE; /* Clear _PAGE_PSE (== _PAGE_PAT) */
-        
         l1_entry = __map_domain_page(pg);
-        for ( i = 0; i < L1_PAGETABLE_ENTRIES; i++ )
+
+        /* Inherit original IOMMU permissions, but update Next Level. */
+        if ( iommu_hap_pt_share )
         {
-            new_entry = l1e_from_pfn(pfn | i, flags);
-            p2m_add_iommu_flags(&new_entry, 0, 0);
-            p2m->write_p2m_entry(p2m, gfn, l1_entry + i, new_entry, 1);
+            flags &= ~iommu_nlevel_to_flags(~0, 0);
+            flags |= iommu_nlevel_to_flags(level - 1, 0);
         }
+
+        for ( i = 0; i < (1u << PAGETABLE_ORDER); i++ )
+        {
+            new_entry = l1e_from_pfn(pfn | (i << ((level - 1) * PAGETABLE_ORDER)),
+                                     flags);
+            p2m->write_p2m_entry(p2m, gfn, l1_entry + i, new_entry, level);
+        }
+
         unmap_domain_page(l1_entry);
-        
+
         new_entry = l1e_from_pfn(mfn_x(page_to_mfn(pg)),
                                  P2M_BASE_FLAGS | _PAGE_RW);
-        p2m_add_iommu_flags(&new_entry, 1, IOMMUF_readable|IOMMUF_writable);
-        p2m->write_p2m_entry(p2m, gfn, p2m_entry, new_entry, 2);
+        p2m_add_iommu_flags(&new_entry, level, IOMMUF_readable|IOMMUF_writable);
+        p2m->write_p2m_entry(p2m, gfn, p2m_entry, new_entry, level + 1);
     }
+    else
+        ASSERT(flags & _PAGE_PRESENT);
 
     next = map_domain_page(_mfn(l1e_get_pfn(*p2m_entry)));
     if ( unmap )