Re: [PATCH for-4.20] x86/PV: further harden guest memory accesses against speculative abuse

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Mon, Jan 27, 2025 at 01:06:51PM +0100, Jan Beulich wrote:
> On 27.01.2025 12:31, Roger Pau Monné wrote:
> > On Mon, Jan 27, 2025 at 11:15:22AM +0100, Jan Beulich wrote:
> >> --- a/xen/arch/x86/include/asm/asm-defns.h
> >> +++ b/xen/arch/x86/include/asm/asm-defns.h
> >> @@ -1,3 +1,5 @@
> >> +#include <asm/page-bits.h>
> >> +
> >>  #ifndef HAVE_AS_CLAC_STAC
> >>  .macro clac
> >>      .byte 0x0f, 0x01, 0xca
> >> @@ -65,17 +67,36 @@
> >>  .macro guest_access_mask_ptr ptr:req, scratch1:req, scratch2:req
> >>  #if defined(CONFIG_SPECULATIVE_HARDEN_GUEST_ACCESS)
> >>      /*
> >> -     * Here we want
> >> -     *
> >> -     * ptr &= ~0ull >> (ptr < HYPERVISOR_VIRT_END);
> >> -     *
> >> +     * Here we want to adjust \ptr such that
> >> +     * - if it's within Xen range, it becomes non-canonical,
> >> +     * - otherwise if it's (non-)canonical on input, it retains that 
> >> property,
> >> +     * - if the result is non-canonical, bit 47 is clear (to avoid
> >> +     *   potentially populating the cache with Xen data),
> > 
> > It's my understanding from the commit message that this toggling of
> > bit 47 is only done due to AMD behavior, as speculative execution
> > there ignores any higher than 47, and hence non-canonical inputs are
> > no detected when speculatively executing?
> > 
> > It might be worth explicitly mentioning this in the comment.
> 
> Hmm, to be honest I'd rather not (and keep mentioning AMD to the
> description). First and foremost because if I mention it here, I
> strictly also ought to mention Hygon, I think. In the description I
> feel a little easier about not specifically saying so. (But yes, if
> you strongly think I should mention vendors here, I'd be okay-ish to
> add "on AMD-like hardware" before the closing paren on the 2nd
> bullet point.)
> 
> Further, with such a consideration, would we perhaps also want to
> consider simplifying the transformation when AMD=n in .config? (I
> could see us doing so, but not as late in a release cycle as we're
> now at. Plus I say so without having thought about whether a
> simplification is actually possible.)
> 
> >>       * but guaranteed without any conditional branches (hence in 
> >> assembly).
> >> +     *
> >> +     * To achieve this we determine which bit to forcibly clear: Either 
> >> bit 47
> >> +     * (in case the address is below HYPERVISOR_VIRT_END) or bit 63.  
> >> Further
> >> +     * we determine whether for forcably set bit 63: In case we first 
> >> cleared
> >> +     * it, we'll merely restore the original address.  In case we ended up
> >> +     * clearing bit 47 (i.e. the address was either non-canonical or 
> >> within Xen
> >> +     * range), setting the bit will yield a guaranteed non-canonical 
> >> address.
> >> +     * If we didn't clear a bit, we also won't set one: The address was 
> >> in the
> >> +     * low half of address space in that case with bit 47 already clear.  
> >> The
> >> +     * address can thus be left unchanged, whether canonical or not.
> > 
> > Since for AMD we have to do the bit 47 clearing, won't it be enough to
> > do something like:
> > 
> > ptr &= (ptr < HYPERVISOR_VIRT_END) ? ~(1ULL <<  (VADDR_BITS - 1) : ~0ULL;
> > 
> > So only care to clear bit 47 when ptr is below HYPERVISOR_VIRT_END?
> > As that would already diverge accesses into the Xen address space
> > without having to toggle bit 63?
> 
> No, this may transform a non-canonical input into a canonical address
> (accesses through which then won't #GP as expected), just for a different
> address range than where we had the same mistake before.

Indeed, you are right.  With the expanded comment:

Reviewed-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>

Thanks, Roger.