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

[Julien Grall <julien@xxxxxxx>]

Hi,

On 04/12/2019 17:10, Hongyan Xia wrote:
> From: Wei Liu <wei.liu2@xxxxxxxxxx>
>
> 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 <wei.liu2@xxxxxxxxxx>
> Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>
> ---
>   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@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel