Re: [PATCH 5/6] x86/mm: clean up SHARED_M2P{, _ENTRY} uses

[George Dunlap <george.dunlap@citrix.com>]

On 12/12/2017 03:08 PM, Jan Beulich wrote:
> Stop open-coding SHARED_M2P() and drop a pointless use of it from
> paging_mfn_is_dirty() (!VALID_M2P() is a superset of SHARED_M2P()) and
> another one from free_page_type() (prior assertions render this
> redundant).
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> --- a/xen/arch/x86/mm.c
> +++ b/xen/arch/x86/mm.c
> @@ -2371,9 +2371,7 @@ int free_page_type(struct page_info *pag
>  
>          gmfn = mfn_to_gmfn(owner, mfn_x(page_to_mfn(page)));
>          ASSERT(VALID_M2P(gmfn));
> -        /* Page sharing not supported for shadowed domains */
> -        if(!SHARED_M2P(gmfn))
> -            shadow_remove_all_shadows(owner, _mfn(gmfn));
> +        shadow_remove_all_shadows(owner, _mfn(gmfn));

But that's an ASSERT(), not a BUG_ON().  Code after an ASSERT() needs to
make sure that if it turns out to be false in a non-debug run, nothing
worse than a BUG() will happen -- for instance, an information leak or a
privilege escalation.

xen/arch/x86/mm/shadow/common.c:sh_remove_shadows() looks up the page
struct for the mfn without checking if it's valid; so it will *probably*
end up accessing a wild pointer; at which point it would be better to
change the ASSERT(VALID_M2P()) into a BUG_ON(!VALID_M2P()).

Or, if we don't want to crash on a production box in that case, we
should leave the if() statement there.

> --- a/xen/arch/x86/mm/paging.c
> +++ b/xen/arch/x86/mm/paging.c
> @@ -369,8 +369,8 @@ int paging_mfn_is_dirty(struct domain *d
>  
>      /* We /really/ mean PFN here, even for non-translated guests. */
>      pfn = _pfn(get_gpfn_from_mfn(mfn_x(gmfn)));
> -    /* Shared pages are always read-only; invalid pages can't be dirty. */
> -    if ( unlikely(SHARED_M2P(pfn_x(pfn)) || !VALID_M2P(pfn_x(pfn))) )
> +    /* Invalid pages can't be dirty. */
> +    if ( unlikely(!VALID_M2P(pfn_x(pfn))) )
>          return 0;

Are you sure that it will always be the case in the future that
SHARED_MP2(x) implies !VALID_M2P(x)?  (This is also relevant for my
previous comment.)

 -George

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