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

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

On Fri, Feb 12, 2021 at 01:48:43PM +0100, Jan Beulich wrote:
> 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.)

I see, it's not easy to build such construct without using any
relational operator. Would you mind adding the C equivalent next to
assembly chunk?

I don't think I have further comments:

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

Thanks, Roger.