Re: [PATCH v6 4/5] x86/mm: Reject invalid cacheability in PV guests by default

[Andrew Cooper <Andrew.Cooper3@xxxxxxxxxx>]

On 22/12/2022 10:31 pm, Demi Marie Obenour wrote:
> diff --git a/docs/misc/xen-command-line.pandoc 
> b/docs/misc/xen-command-line.pandoc
> index 
> 424b12cfb27d6ade2ec63eacb8afe5df82465451..0230a7bc17cbd4362a42ea64cea695f31f5e0f86
>  100644
> --- a/docs/misc/xen-command-line.pandoc
> +++ b/docs/misc/xen-command-line.pandoc
> @@ -1417,6 +1417,17 @@ detection of systems known to misbehave upon accesses 
> to that port.
>  ### idle_latency_factor (x86)
>  > `= <integer>`
>  
> +### invalid-cacheability (x86)
> +> `= allow | deny | trap`
> +
> +> Default: `deny` in release builds, otherwise `trap`
> +
> +Specify what happens when a PV guest tries to use one of the reserved 
> entries in
> +the PAT.  `deny` causes the attempt to be rejected with -EINVAL, `allow` 
> allows
> +the attempt, and `trap` causes a general protection fault to be raised.
> +Currently, the reserved entries are marked as uncacheable in Xen's PAT, but 
> this
> +will change if new memory types are added, so guests must not rely on it.
> +

This wants to be scoped under "pv", and not a top-level option.  Current
parsing is at the top of xen/arch/x86/pv/domain.c

How about `pv={no-}oob-pat`, and parse it into the opt_pv_oob_pat boolean ?

There really is no need to distinguish between deny and trap.  IMO,
injecting #GP unilaterally is fine (to a guest expecting the hypercall
to succeed, -EINVAL vs #GP makes no difference, but #GP is far more
useful to a human trying to debug issues here), but I could be talked
into putting that behind a CONFIG_DEBUG if other feel strongly.

> @@ -1343,7 +1374,34 @@ static int promote_l1_table(struct page_info *page)
>          }
>          else
>          {
> -            switch ( ret = get_page_from_l1e(pl1e[i], d, d) )
> +            l1_pgentry_t l1e = pl1e[i];
> +
> +            if ( invalid_cacheability != INVALID_CACHEABILITY_ALLOW )
> +            {
> +                switch ( l1e.l1 & PAGE_CACHE_ATTRS )
> +                {
> +                case _PAGE_WB:
> +                case _PAGE_UC:
> +                case _PAGE_UCM:
> +                case _PAGE_WC:
> +                case _PAGE_WT:
> +                case _PAGE_WP:
> +                    break;
> +                default:
> +                    /*
> +                     * If we get here, a PV guest tried to use one of the
> +                     * reserved values in Xen's PAT.  This indicates a bug
> +                     * in the guest.  If requested by the user, inject #GP
> +                     * to cause the guest to log a stack trace.
> +                     */
> +                    if ( invalid_cacheability == INVALID_CACHEABILITY_TRAP )
> +                        pv_inject_hw_exception(TRAP_gp_fault, 0);
> +                    ret = -EINVAL;
> +                    goto fail;
> +                }
> +            }

This will catch cases where the guest writes a full pagetable, then
promotes it, but won't catch cases where the guest modifies an
individual entry with mmu_update/etc.

The logic needs to be in get_page_from_l1e() to get applied uniformly to
all PTE modifications.

Right now, the l1_disallow_mask() check near the start hides the "can
you use a nonzero cacheattr" check.  If I ever get around to cleaning up
my post-XSA-402 series, I have a load of modifications to this.

But this could be something like this:

if ( opt_pv_oob_pat && (l1f & PAGE_CACHE_ATTRS) > _PAGE_WP )
{
    // #GP
    return -EINVAL;
}

fairly early on.

It occurs to me that this check is only applicable when we're using the
Xen ABI APT, and seeing as we've talked about possibly making patch 5 a
Kconfig option, that may want accounting here.  (Perhaps simply making
opt_pb_oob_pat be false in a !XEN_PAT build.)

~Andrew