Re: [PATCH 04/17] x86/PV: harden guest memory accesses against speculative abuse

[Jan Beulich <jbeulich@xxxxxxxx>]

On 12.02.2021 11:41, Roger Pau Monné wrote:
> On Thu, Jan 14, 2021 at 04:04:57PM +0100, Jan Beulich wrote:
>> @@ -94,6 +106,8 @@ unsigned __copy_from_user_ll(void *to, c
>>      return n;
>>  }
>>  
>> +#if GUARD(1) + 0
> 
> Why do you need the '+ 0' here? I guess it's to prevent the
> preprocessor from complaining when GUARD(1) gets replaced by nothing?

Yes. "#if" with nothing after it is an error from all I know.

>> --- a/xen/include/asm-x86/asm-defns.h
>> +++ b/xen/include/asm-x86/asm-defns.h
>> @@ -44,3 +44,16 @@
>>  .macro INDIRECT_JMP arg:req
>>      INDIRECT_BRANCH jmp \arg
>>  .endm
>> +
>> +.macro guest_access_mask_ptr ptr:req, scratch1:req, scratch2:req
>> +#if defined(CONFIG_SPECULATIVE_HARDEN_GUEST_ACCESS)
>> +    mov $(HYPERVISOR_VIRT_END - 1), \scratch1
>> +    mov $~0, \scratch2
>> +    cmp \ptr, \scratch1
>> +    rcr $1, \scratch2
>> +    and \scratch2, \ptr
> 
> If my understanding is correct, that's equivalent to:
> 
> ptr &= ~0ull >> (ptr < HYPERVISOR_VIRT_END);
> 
> It might be helpful to add this as a comment, to clarify the indented
> functionality of the assembly bit.
> 
> I wonder if the C code above can generate any jumps? As you pointed
> out, we already use something similar in array_index_mask_nospec and
> that's fine to do in C.

Note how array_index_mask_nospec() gets away without any use of
relational operators. They're what poses the risk of getting
translated to branches. (Quite likely the compiler wouldn't use
any in the case here, as the code can easily get away without,
but we don't want to chance it. Afaict it would instead use a
3rd scratch register, so register pressure might still lead to
using a branch instead in some exceptional case.)

Jan