[Xen-devel] [PATCH v4 4/9] x86/mm: introduce l{1, 2}t local variables to modify_xen_mappings

2019-12-04 Thread Hongyan Xia 
From: Wei Liu 

The pl2e and pl1e variables are heavily (ab)used in that function.  It
is fine at the moment because all page tables are always mapped so
there is no need to track the life time of each variable.

We will soon have the requirement to map and unmap page tables. We
need to track the life time of each variable to avoid leakage.

Introduce some l{1,2}t variables with limited scope so that we can
track life time of pointers to xen page tables more easily.

No functional change.

Signed-off-by: Wei Liu 
Reviewed-by: Jan Beulich 
---
 xen/arch/x86/mm.c | 68 ++-
 1 file changed, 38 insertions(+), 30 deletions(-)

diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
index 790578d2b3..303bc35549 100644
--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -5601,6 +5601,8 @@ int modify_xen_mappings(unsigned long s, unsigned long e, 
unsigned int nf)
 
 if ( l3e_get_flags(*pl3e) & _PAGE_PSE )
 {
+l2_pgentry_t *l2t;
+
 if ( l2_table_offset(v) == 0 &&
  l1_table_offset(v) == 0 &&
  ((e - v) >= (1UL << L3_PAGETABLE_SHIFT)) )
@@ -5616,11 +5618,11 @@ int modify_xen_mappings(unsigned long s, unsigned long 
e, unsigned int nf)
 }
 
 /* PAGE1GB: shatter the superpage and fall through. */
-pl2e = alloc_xen_pagetable();
-if ( !pl2e )
+l2t = alloc_xen_pagetable();
+if ( !l2t )
 return -ENOMEM;
 for ( i = 0; i < L2_PAGETABLE_ENTRIES; i++ )
-l2e_write(pl2e + i,
+l2e_write(l2t + i,
   l2e_from_pfn(l3e_get_pfn(*pl3e) +
(i << PAGETABLE_ORDER),
l3e_get_flags(*pl3e)));
@@ -5629,14 +5631,14 @@ int modify_xen_mappings(unsigned long s, unsigned long 
e, unsigned int nf)
 if ( (l3e_get_flags(*pl3e) & _PAGE_PRESENT) &&
  (l3e_get_flags(*pl3e) & _PAGE_PSE) )
 {
-l3e_write_atomic(pl3e, l3e_from_mfn(virt_to_mfn(pl2e),
+l3e_write_atomic(pl3e, l3e_from_mfn(virt_to_mfn(l2t),
 __PAGE_HYPERVISOR));
-pl2e = NULL;
+l2t = NULL;
 }
 if ( locking )
 spin_unlock(&map_pgdir_lock);
-if ( pl2e )
-free_xen_pagetable(pl2e);
+if ( l2t )
+free_xen_pagetable(l2t);
 }
 
 /*
@@ -5670,12 +5672,14 @@ int modify_xen_mappings(unsigned long s, unsigned long 
e, unsigned int nf)
 }
 else
 {
+l1_pgentry_t *l1t;
+
 /* PSE: shatter the superpage and try again. */
-pl1e = alloc_xen_pagetable();
-if ( !pl1e )
+l1t = alloc_xen_pagetable();
+if ( !l1t )
 return -ENOMEM;
 for ( i = 0; i < L1_PAGETABLE_ENTRIES; i++ )
-l1e_write(&pl1e[i],
+l1e_write(&l1t[i],
   l1e_from_pfn(l2e_get_pfn(*pl2e) + i,
l2e_get_flags(*pl2e) & ~_PAGE_PSE));
 if ( locking )
@@ -5683,19 +5687,19 @@ int modify_xen_mappings(unsigned long s, unsigned long 
e, unsigned int nf)
 if ( (l2e_get_flags(*pl2e) & _PAGE_PRESENT) &&
  (l2e_get_flags(*pl2e) & _PAGE_PSE) )
 {
-l2e_write_atomic(pl2e, l2e_from_mfn(virt_to_mfn(pl1e),
+l2e_write_atomic(pl2e, l2e_from_mfn(virt_to_mfn(l1t),
 __PAGE_HYPERVISOR));
-pl1e = NULL;
+l1t = NULL;
 }
 if ( locking )
 spin_unlock(&map_pgdir_lock);
-if ( pl1e )
-free_xen_pagetable(pl1e);
+if ( l1t )
+free_xen_pagetable(l1t);
 }
 }
 else
 {
-l1_pgentry_t nl1e;
+l1_pgentry_t nl1e, *l1t;
 
 /*
  * Ordinary 4kB mapping: The L2 entry has been verified to be
@@ -5742,9 +5746,9 @@ int modify_xen_mappings(unsigned long s, unsigned long e, 
unsigned int nf)
 continue;
 }
 
-pl1e = l2e_to_l1e(*pl2e);
+l1t = l2e_to_l1e(*pl2e);
 for ( i = 0; i < L1_PAGETABLE_ENTRIES; i++ )
-if ( l1e_get_intpte(pl1e[i]) != 0 )
+if ( l1e_get_intpte(l1t[i]) != 0 )
 break;
 if ( i == L1_PAGETABLE_ENTRIES )
 {
@@ -5753,7 +5757,7 @@ int modify_xen_mappings(unsigned long s, unsigned long e, 
unsigned int nf)
 if ( locking )
 spin_unlock(&map_pgdir_lock

Re: [Xen-devel] [PATCH v4 4/9] x86/mm: introduce l{1, 2}t local variables to modify_xen_mappings

2019-12-12 Thread Julien Grall 

Hi,

On 04/12/2019 17:10, Hongyan Xia wrote:

From: Wei Liu 

The pl2e and pl1e variables are heavily (ab)used in that function.  It
is fine at the moment because all page tables are always mapped so
there is no need to track the life time of each variable.

We will soon have the requirement to map and unmap page tables. We
need to track the life time of each variable to avoid leakage.

Introduce some l{1,2}t variables with limited scope so that we can
track life time of pointers to xen page tables more easily.

No functional change.

Signed-off-by: Wei Liu 
Reviewed-by: Jan Beulich 
---
  xen/arch/x86/mm.c | 68 ++-
  1 file changed, 38 insertions(+), 30 deletions(-)

diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
index 790578d2b3..303bc35549 100644
--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -5601,6 +5601,8 @@ int modify_xen_mappings(unsigned long s, unsigned long e, 
unsigned int nf)
  
  if ( l3e_get_flags(*pl3e) & _PAGE_PSE )

  {
+l2_pgentry_t *l2t;
+
  if ( l2_table_offset(v) == 0 &&
   l1_table_offset(v) == 0 &&
   ((e - v) >= (1UL << L3_PAGETABLE_SHIFT)) )
@@ -5616,11 +5618,11 @@ int modify_xen_mappings(unsigned long s, unsigned long 
e, unsigned int nf)
  }
  
  /* PAGE1GB: shatter the superpage and fall through. */

-pl2e = alloc_xen_pagetable();
-if ( !pl2e )
+l2t = alloc_xen_pagetable();
+if ( !l2t )
  return -ENOMEM;


Indirectly related to this patch, it looks like TLBs will not be flushed 
even part of the mapping is not removed.


Another problem I have spotted is most of the callers of 
map_pages_to_xen() & modify_xen_mappings() will never check the return 
value.


If we plan to use destroy_xen_mappings() for unmapping xenheap page, 
then we will need to ensure that destroy_xen_mappings() will never fail. 
Otherwise we will end up to keep part of the mappings and therefore 
defeating the purpose of secret hiding.


This may mean that shattering/merging should be prevented for xenheap 
region.


Cheers,

--
Julien Grall

___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel