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

["Jan Beulich" <JBeulich@suse.com>]

>>> On 19.01.18 at 15:24, <andrew.cooper3@citrix.com> wrote:
> On 19/01/18 12:47, Jan Beulich wrote:
>>>>> On 18.01.18 at 16:46, <andrew.cooper3@citrix.com> 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

Jan

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