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
