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

[Jan Beulich <jbeulich@xxxxxxxx>]

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.

Jan