Re: [PATCH 4/8] x86/mm: Drop bogus cacheability logic in update_xen_mappings()

[Andrew Cooper <amc96@xxxxxxxx>]

On 30/11/2021 10:04, Andrew Cooper wrote:
> There is no circumstance under which any part of the Xen image in memory wants
> to have any cacheability other than Write Back.
>
> Furthermore, unmapping random pieces of Xen like that comes with a non-trivial
> risk of a Triple Fault, or other fatal condition.  Even if it appears to work,
> an NMI or interrupt as a far wider reach into Xen mappings than calling
> map_pages_to_xen() thrice.
>
> Simplfy the safety checking to a BUG_ON().  It is substantially more correct
> and robust than following either of the unlikely(alias) paths.
>
> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> ---
> CC: Jan Beulich <JBeulich@xxxxxxxx>
> CC: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> CC: Wei Liu <wl@xxxxxxx>
>
> I'm half tempted to drop the check entirely, but in that case it would be
> better to inline the result into the two callers.
> ---
>  xen/arch/x86/mm.c | 21 +++++++++------------
>  1 file changed, 9 insertions(+), 12 deletions(-)
>
> diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
> index 4d799032dc82..9bd4e5cc1d2f 100644
> --- a/xen/arch/x86/mm.c
> +++ b/xen/arch/x86/mm.c
> @@ -785,24 +785,21 @@ bool is_iomem_page(mfn_t mfn)
>  
>  static int update_xen_mappings(unsigned long mfn, unsigned int cacheattr)
>  {
> -    int err = 0;
>      bool alias = mfn >= PFN_DOWN(xen_phys_start) &&
>           mfn < PFN_UP(xen_phys_start + xen_virt_end - XEN_VIRT_START);
> -    unsigned long xen_va =
> -        XEN_VIRT_START + ((mfn - PFN_DOWN(xen_phys_start)) << PAGE_SHIFT);
> +
> +    /*
> +     * Something is catastrophically broken if someone is trying to change 
> the
> +     * cacheability of Xen in memory...
> +     */
> +    BUG_ON(alias);
>  
>      if ( boot_cpu_has(X86_FEATURE_XEN_SELFSNOOP) )
>          return 0;
>  
> -    if ( unlikely(alias) && cacheattr )
> -        err = map_pages_to_xen(xen_va, _mfn(mfn), 1, 0);
> -    if ( !err )
> -        err = map_pages_to_xen((unsigned long)mfn_to_virt(mfn), _mfn(mfn), 1,
> -                     PAGE_HYPERVISOR | cacheattr_to_pte_flags(cacheattr));
> -    if ( unlikely(alias) && !cacheattr && !err )
> -        err = map_pages_to_xen(xen_va, _mfn(mfn), 1, PAGE_HYPERVISOR);
> -
> -    return err;
> +    return map_pages_to_xen(
> +        (unsigned long)mfn_to_virt(mfn), _mfn(mfn), 1,
> +        PAGE_HYPERVISOR | cacheattr_to_pte_flags(cacheattr));

In light of the discussion on patch 1, this is no longer safe.  The
alias calculation includes 2M of arbitrary guest memory, and in
principle there are legitimate reasons for a guest to want to map RAM as
WC (e.g. GPU pagetables) or in the future, WP (in place RAM
encryption/decryption).

The gross hack fix would be "mfn >= PFN_DOWN(xen_phys_start + MB(2))",
but but this is screaming for a helper.  xen_in_range() is part-way
there, but is an O(n) loop over the regions.

~Andrew