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

From: Julien Grall <julien@xen.org>
To: Hongyan Xia <hongyxia@amazon.com>, xen-devel@lists.xenproject.org
Cc: "Andrew Cooper" <andrew.cooper3@citrix.com>,
	"Jan Beulich" <jbeulich@suse.com>, "Wei Liu" <wl@xen.org>,
	"Roger Pau Monné" <roger.pau@citrix.com>
Subject: Re: [Xen-devel] [PATCH v4 4/9] x86/mm: introduce l{1, 2}t local variables to modify_xen_mappings
Date: Thu, 12 Dec 2019 14:34:21 +0000	[thread overview]
Message-ID: <2429d27e-d4df-6aeb-40db-119a30572d4d@xen.org> (raw)
In-Reply-To: <b23924c9bdfe076c970dad4cbd9fa4d946d0a168.1575477921.git.hongyxia@amazon.com>

Hi,

On 04/12/2019 17:10, Hongyan Xia wrote:
> From: Wei Liu <wei.liu2@citrix.com>
> 
> 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@citrix.com>
> Reviewed-by: Jan Beulich <jbeulich@suse.com>
> ---
>   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