Re: [Xen-devel] [PATCH v9 08/11] x86/entry: Clobber the Return Stack Buffer/Return Address Stack on entry to Xen

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 19/01/18 15:02, Jan Beulich wrote:
>>>> On 19.01.18 at 15:24, <andrew.cooper3@xxxxxxxxxx> wrote:
>> On 19/01/18 12:47, Jan Beulich wrote:
>>>>>> On 18.01.18 at 16:46, <andrew.cooper3@xxxxxxxxxx> wrote:
>>>> @@ -265,6 +265,10 @@ On hardware supporting IBRS, the `ibrs=` option can 
>>>> be 
>>>> used to force or
>>>>  prevent Xen using the feature itself.  If Xen is not using IBRS itself,
>>>>  functionality is still set up so IBRS can be virtualised for guests.
>>>>  
>>>> +The `rsb_vmexit=` and `rsb_native=` options can be used to fine tune when 
>>>> the
>>>> +RSB gets overwritten.  There are individual controls for an entry from HVM
>>>> +context, and an entry from a native (PV or Xen) context.
>>> Would you mind adding a sentence or two to the description making
>>> clear what use this fine grained control is? I can't really figure why I
>>> might need to be concerned about one of the two cases, but not the
>>> other.
>> I though I'd covered that in the commit message, but I'm not sure this
>> is a suitable place to discuss the details.  PV and HVM guests have
>> different reasoning for why we need to overwrite the RSB.
>>
>> In the past, there used to be a default interaction of rsb_native and
>> SMEP, but that proved to be insufficient and rsb_native is now
>> unconditionally enabled.  In principle however, it should fall within
>> CONFIG_PV.
> Thanks for the explanation, but I'm afraid I'm none the wiser as
> to why the two separate options are needed (or even just wanted).
>
>>>> --- a/xen/include/asm-x86/spec_ctrl_asm.h
>>>> +++ b/xen/include/asm-x86/spec_ctrl_asm.h
>>>> @@ -73,6 +73,40 @@
>>>>   *  - SPEC_CTRL_EXIT_TO_GUEST
>>>>   */
>>>>  
>>>> +.macro DO_OVERWRITE_RSB
>>>> +/*
>>>> + * Requires nothing
>>>> + * Clobbers %rax, %rcx
>>>> + *
>>>> + * Requires 256 bytes of stack space, but %rsp has no net change. Based on
>>>> + * Google's performance numbers, the loop is unrolled to 16 iterations 
>>>> and two
>>>> + * calls per iteration.
>>>> + *
>>>> + * The call filling the RSB needs a nonzero displacement, but we use "1:
>>>> + * pause, jmp 1b" to safely contains any ret-based speculation, even if 
>>>> the
>>>> + * loop is speculatively executed prematurely.
>>> I'm struggling to understand why you use "but" here. Maybe just a
>>> lack of English skills on my part?
>> "displacement.  A nop would do, but" ?
>>
>> It is a justification for why we are putting more than a single byte in
>> the middle.
> Oh, I see, but only with the addition you suggest.
>
>>>> + * %rsp is preserved by using an extra GPR because a) we've got plenty 
>>>> spare,
>>>> + * b) the two movs are shorter to encode than `add $32*8, %rsp`, and c) 
>>>> can be
>>>> + * optimised with mov-elimination in modern cores.
>>>> + */
>>>> +    mov $16, %ecx   /* 16 iterations, two calls per loop */
>>>> +    mov %rsp, %rax  /* Store the current %rsp */
>>>> +
>>>> +.L\@_fill_rsb_loop:
>>>> +
>>>> +    .rept 2         /* Unrolled twice. */
>>>> +    call 2f         /* Create an RSB entry. */
>>>> +1:  pause
>>>> +    jmp 1b          /* Capture rogue speculation. */
>>>> +2:
>>> I won't further insist on changing away from numeric labels here, but
>>> I'd still like to point out an example of a high risk use of such labels in
>>> mainline code: There's a "jz 1b" soon after
>>> exception_with_ints_disabled, leading across _two_ other labels and
>>> quite a few insns and macro invocations. May I at the very least
>>> suggest that you don't use 1 and 2 here?
>> I spent ages trying to get .L labels working here, but they don't
>> function inside a rept, as you end up with duplicate local symbols.
>>
>> Even using irp to inject a unique number into the loop doesn't appear to
>> work, because the \ escape gets interpreted as a token separator. 
>> AFAICT, \@ is special by virtue of the fact that it doesn't count as a
>> token separator.
>>
>> If you've got a better suggestion then I'm all ears.
>>
>> Alternatively, I could manually unroll the loop, or pick some arbitrary
>> other numbers to use.
> Since the unroll number is just 2, this is what I would have
> suggested primarily. .rept of course won't work, as it's not a
> macro invocation, and hence doesn't increment the internal
> counter. With .irp I can get things to work:
>
>       .macro m
>       .irp n, 1, 2
> .Lxyz_\@_\n:  mov     $\@, %eax
>       .endr
>       .endm

This appears to only work when \n is at the end of the label.  None of:

.Lxyz_\@_\n_:    mov    $\@, %eax
.Lxyz_\@_\n\()_:    mov    $\@, %eax
.Lxyz_\n\@:    mov    $\@, %eax

work.

Given this appears to be a corner case to begin with, how likely do you
think it is to work with older assemblers?

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel