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
