Each IDT vector needs to land on an endbr64 instruction.  This is especially
important for the #CP handler, which will escalate to #DF if the endbr64 is
missing.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Wei Liu <wl@xen.org>
---
 xen/arch/x86/x86_64/compat/entry.S |  1 +
 xen/arch/x86/x86_64/entry.S        | 28 ++++++++++++++++++++++++++--
 2 files changed, 27 insertions(+), 2 deletions(-)

diff --git a/xen/arch/x86/x86_64/compat/entry.S b/xen/arch/x86/x86_64/compat/entry.S
index c84ff7ea6476..5fd6dbbd4513 100644
--- a/xen/arch/x86/x86_64/compat/entry.S
+++ b/xen/arch/x86/x86_64/compat/entry.S
@@ -12,6 +12,7 @@
 #include <irq_vectors.h>
 
 ENTRY(entry_int82)
+        ENDBR64
         ALTERNATIVE "", clac, X86_FEATURE_XEN_SMAP
         pushq $0
         movl  $HYPERCALL_VECTOR, 4(%rsp)
diff --git a/xen/arch/x86/x86_64/entry.S b/xen/arch/x86/x86_64/entry.S
index 9abcf95bd010..c5fa4b3c0f41 100644
--- a/xen/arch/x86/x86_64/entry.S
+++ b/xen/arch/x86/x86_64/entry.S
@@ -386,6 +386,7 @@ UNLIKELY_END(sysenter_gpf)
         jmp   .Lbounce_exception
 
 ENTRY(int80_direct_trap)
+        ENDBR64
         ALTERNATIVE "", clac, X86_FEATURE_XEN_SMAP
         pushq $0
         movl  $0x80, 4(%rsp)
@@ -698,6 +699,7 @@ ENTRY(common_interrupt)
         jmp ret_from_intr
 
 ENTRY(page_fault)
+        ENDBR64
         movl  $TRAP_page_fault,4(%rsp)
 /* No special register assumptions. */
 GLOBAL(handle_exception)
@@ -872,75 +874,91 @@ FATAL_exception_with_ints_disabled:
         BUG   /* fatal_trap() shouldn't return. */
 
 ENTRY(divide_error)
+        ENDBR64
         pushq $0
         movl  $TRAP_divide_error,4(%rsp)
         jmp   handle_exception
 
 ENTRY(coprocessor_error)
+        ENDBR64
         pushq $0
         movl  $TRAP_copro_error,4(%rsp)
         jmp   handle_exception
 
 ENTRY(simd_coprocessor_error)
+        ENDBR64
         pushq $0
         movl  $TRAP_simd_error,4(%rsp)
         jmp   handle_exception
 
 ENTRY(device_not_available)
+        ENDBR64
         pushq $0
         movl  $TRAP_no_device,4(%rsp)
         jmp   handle_exception
 
 ENTRY(debug)
+        ENDBR64
         pushq $0
         movl  $TRAP_debug,4(%rsp)
         jmp   handle_ist_exception
 
 ENTRY(int3)
+        ENDBR64
         pushq $0
         movl  $TRAP_int3,4(%rsp)
         jmp   handle_exception
 
 ENTRY(overflow)
+        ENDBR64
         pushq $0
         movl  $TRAP_overflow,4(%rsp)
         jmp   handle_exception
 
 ENTRY(bounds)
+        ENDBR64
         pushq $0
         movl  $TRAP_bounds,4(%rsp)
         jmp   handle_exception
 
 ENTRY(invalid_op)
+        ENDBR64
         pushq $0
         movl  $TRAP_invalid_op,4(%rsp)
         jmp   handle_exception
 
 ENTRY(invalid_TSS)
+        ENDBR64
         movl  $TRAP_invalid_tss,4(%rsp)
         jmp   handle_exception
 
 ENTRY(segment_not_present)
+        ENDBR64
         movl  $TRAP_no_segment,4(%rsp)
         jmp   handle_exception
 
 ENTRY(stack_segment)
+        ENDBR64
         movl  $TRAP_stack_error,4(%rsp)
         jmp   handle_exception
 
 ENTRY(general_protection)
+        ENDBR64
         movl  $TRAP_gp_fault,4(%rsp)
         jmp   handle_exception
 
 ENTRY(alignment_check)
+        ENDBR64
         movl  $TRAP_alignment_check,4(%rsp)
         jmp   handle_exception
 
 ENTRY(entry_CP)
+        ENDBR64
         movl  $X86_EXC_CP, 4(%rsp)
         jmp   handle_exception
 
 ENTRY(double_fault)
+        ENDBR64
         movl  $TRAP_double_fault,4(%rsp)
         /* Set AC to reduce chance of further SMAP faults */
         ALTERNATIVE "", stac, X86_FEATURE_XEN_SMAP
@@ -966,6 +984,7 @@ ENTRY(double_fault)
 
         .pushsection .init.text, "ax", @progbits
 ENTRY(early_page_fault)
+        ENDBR64
         movl  $TRAP_page_fault,4(%rsp)
         SAVE_ALL
         movq  %rsp,%rdi
@@ -974,6 +993,7 @@ ENTRY(early_page_fault)
         .popsection
 
 ENTRY(nmi)
+        ENDBR64
         pushq $0
         movl  $TRAP_nmi,4(%rsp)
 handle_ist_exception:
@@ -1102,12 +1122,14 @@ handle_ist_exception:
 #endif
 
 ENTRY(machine_check)
+        ENDBR64
         pushq $0
         movl  $TRAP_machine_check,4(%rsp)
         jmp   handle_ist_exception
 
 /* No op trap handler.  Required for kexec crash path. */
 GLOBAL(trap_nop)
+        ENDBR64
         iretq
 
 /* Table of automatically generated entry points.  One per vector. */
@@ -1136,7 +1158,8 @@ autogen_stubs: /* Automatically generated stubs. */
 #endif
 
         ALIGN
-1:      pushq $0
+1:      ENDBR64
+        pushq $0
         movb  $vec,4(%rsp)
         jmp   common_interrupt
 
@@ -1146,7 +1169,8 @@ autogen_stubs: /* Automatically generated stubs. */
         .elseif vec == X86_EXC_CSO || vec == X86_EXC_SPV || \
                 vec == X86_EXC_VE  || (vec > X86_EXC_CP && vec < TRAP_nr)
 
-1:      test  $8,%spl        /* 64bit exception frames are 16 byte aligned, but the word */
+1:      ENDBR64
+        test  $8,%spl        /* 64bit exception frames are 16 byte aligned, but the word */
         jz    2f             /* size is 8 bytes.  Check whether the processor gave us an */
         pushq $0             /* error code, and insert an empty one if not.              */
 2:      movb  $vec,4(%rsp)
-- 
2.11.0

On 26.11.2021 13:34, Andrew Cooper wrote:
> Each IDT vector needs to land on an endbr64 instruction.  This is especially
> important for the #CP handler, which will escalate to #DF if the endbr64 is
> missing.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Jan Beulich <jbeulich@suse.com>

One remark though:

> @@ -1136,7 +1158,8 @@ autogen_stubs: /* Automatically generated stubs. */
>  #endif
>  
>          ALIGN
> -1:      pushq $0
> +1:      ENDBR64
> +        pushq $0
>          movb  $vec,4(%rsp)
>          jmp   common_interrupt
>  
> @@ -1146,7 +1169,8 @@ autogen_stubs: /* Automatically generated stubs. */
>          .elseif vec == X86_EXC_CSO || vec == X86_EXC_SPV || \
>                  vec == X86_EXC_VE  || (vec > X86_EXC_CP && vec < TRAP_nr)
>  
> -1:      test  $8,%spl        /* 64bit exception frames are 16 byte aligned, but the word */
> +1:      ENDBR64
> +        test  $8,%spl        /* 64bit exception frames are 16 byte aligned, but the word */
>          jz    2f             /* size is 8 bytes.  Check whether the processor gave us an */
>          pushq $0             /* error code, and insert an empty one if not.              */
>  2:      movb  $vec,4(%rsp)

Like with initializers of compound objects vs trailing commas there, I
think it would help if we moved away from placing insns on the same
lines as labels. As can be seen here, inserting something always means
touching two lines instead of just adding one.

Jan

On 03/12/2021 13:23, Jan Beulich wrote:
> On 26.11.2021 13:34, Andrew Cooper wrote:
>> Each IDT vector needs to land on an endbr64 instruction.  This is especially
>> important for the #CP handler, which will escalate to #DF if the endbr64 is
>> missing.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Reviewed-by: Jan Beulich <jbeulich@suse.com>

Thanks.

>
> One remark though:
>
>> @@ -1136,7 +1158,8 @@ autogen_stubs: /* Automatically generated stubs. */
>>  #endif
>>  
>>          ALIGN
>> -1:      pushq $0
>> +1:      ENDBR64
>> +        pushq $0
>>          movb  $vec,4(%rsp)
>>          jmp   common_interrupt
>>  
>> @@ -1146,7 +1169,8 @@ autogen_stubs: /* Automatically generated stubs. */
>>          .elseif vec == X86_EXC_CSO || vec == X86_EXC_SPV || \
>>                  vec == X86_EXC_VE  || (vec > X86_EXC_CP && vec < TRAP_nr)
>>  
>> -1:      test  $8,%spl        /* 64bit exception frames are 16 byte aligned, but the word */
>> +1:      ENDBR64
>> +        test  $8,%spl        /* 64bit exception frames are 16 byte aligned, but the word */
>>          jz    2f             /* size is 8 bytes.  Check whether the processor gave us an */
>>          pushq $0             /* error code, and insert an empty one if not.              */
>>  2:      movb  $vec,4(%rsp)
> Like with initializers of compound objects vs trailing commas there, I
> think it would help if we moved away from placing insns on the same
> lines as labels. As can be seen here, inserting something always means
> touching two lines instead of just adding one.

I had actually wondered the same, without drawing a comparison to
trailing commas.  I'll adjust.

~Andrew

On 26.11.2021 13:34, Andrew Cooper wrote:
> Each IDT vector needs to land on an endbr64 instruction.  This is especially
> important for the #CP handler, which will escalate to #DF if the endbr64 is
> missing.

One question here: How does this work? I don't recall there being any "CET
shadow" along the lines of "STI shadow" and "SS shadow", yet there's
clearly an insn boundary here that gets "skipped" if the 2nd #CP gets
converted to #DF. And fetching of the first handler insn also isn't part
of exception delivery (and could cause other exceptions first, like #PF).

Jan

On 03/12/2021 13:32, Jan Beulich wrote:
> On 26.11.2021 13:34, Andrew Cooper wrote:
>> Each IDT vector needs to land on an endbr64 instruction.  This is especially
>> important for the #CP handler, which will escalate to #DF if the endbr64 is
>> missing.
> One question here: How does this work?

Honestly, I'm not sure.

>  I don't recall there being any "CET
> shadow" along the lines of "STI shadow" and "SS shadow", yet there's
> clearly an insn boundary here that gets "skipped" if the 2nd #CP gets
> converted to #DF. And fetching of the first handler insn also isn't part
> of exception delivery (and could cause other exceptions first, like #PF).

I can't make my observations of real hardware behaviour match the
description in the spec.

Given what a mess it all is, I wouldn't be surprised if the exception
delivery microcode has a special case to escalate this to #DF.

If it didn't escalate to #DF, then you'd end up with an infinite stream
of #CP's, which will most likely cause a stack overflow because #CP
needs to be not-IST for shadow stack reasons.

~Andrew

On 03.12.2021 16:30, Andrew Cooper wrote:
> On 03/12/2021 13:32, Jan Beulich wrote:
>> On 26.11.2021 13:34, Andrew Cooper wrote:
>>> Each IDT vector needs to land on an endbr64 instruction.  This is especially
>>> important for the #CP handler, which will escalate to #DF if the endbr64 is
>>> missing.
>> One question here: How does this work?
> 
> Honestly, I'm not sure.
> 
>>  I don't recall there being any "CET
>> shadow" along the lines of "STI shadow" and "SS shadow", yet there's
>> clearly an insn boundary here that gets "skipped" if the 2nd #CP gets
>> converted to #DF. And fetching of the first handler insn also isn't part
>> of exception delivery (and could cause other exceptions first, like #PF).
> 
> I can't make my observations of real hardware behaviour match the
> description in the spec.

I haven't been able to find a description at all of exception behavior
when the exception occurs in wait-for-endbr state. There is text saying
that #BP and #DB can occur this way, but I couldn't find anything about
the tracker state changes in such cases. While I could see the state to
remain engaged (requiring an ENDBR at the handler's entry point), I
cannot see how the state would get re-engaged upon IRET from the
exception handler, unless the return is back to CPL3.

> Given what a mess it all is, I wouldn't be surprised if the exception
> delivery microcode has a special case to escalate this to #DF.

I am meanwhile wondering whether any exception in wait-for-endbr state
at CPL < 3 would promote to #DF, for loss of state. Albeit there must
still be a distinction between CALL/JMP induced state and that
resulting from interrupt or exception delivery. Yet there's no
architectural (or shadow) state expressing "first insn of an exception
handler".

I'm not even convinced the aforementioned statements about #DB and #BP
are actually meant to cover more than just CPL3, or at best ENDBR at
normal CALL/JMP destinations.

While for Xen's own use we may get away without knowing how all of this
actually works (perhaps accepting the fact that one can't set breakpoints
at exception handler entry points, depending on whether their delivery
would promote to #DF), as soon as we were to support CET-IBT for guests
we'd definitely need to know.

Jan

> If it didn't escalate to #DF, then you'd end up with an infinite stream
> of #CP's, which will most likely cause a stack overflow because #CP
> needs to be not-IST for shadow stack reasons.
> 
> ~Andrew
> 

On 06/12/2021 09:42, Jan Beulich wrote:
> On 03.12.2021 16:30, Andrew Cooper wrote:
>> On 03/12/2021 13:32, Jan Beulich wrote:
>>> On 26.11.2021 13:34, Andrew Cooper wrote:
>>>> Each IDT vector needs to land on an endbr64 instruction.  This is especially
>>>> important for the #CP handler, which will escalate to #DF if the endbr64 is
>>>> missing.
>>> One question here: How does this work?
>> Honestly, I'm not sure.
>>
>>>  I don't recall there being any "CET
>>> shadow" along the lines of "STI shadow" and "SS shadow", yet there's
>>> clearly an insn boundary here that gets "skipped" if the 2nd #CP gets
>>> converted to #DF. And fetching of the first handler insn also isn't part
>>> of exception delivery (and could cause other exceptions first, like #PF).
>> I can't make my observations of real hardware behaviour match the
>> description in the spec.
> I haven't been able to find a description at all of exception behavior
> when the exception occurs in wait-for-endbr state. There is text saying
> that #BP and #DB can occur this way, but I couldn't find anything about
> the tracker state changes in such cases. While I could see the state to
> remain engaged (requiring an ENDBR at the handler's entry point), I
> cannot see how the state would get re-engaged upon IRET from the
> exception handler, unless the return is back to CPL3.

Critically, there are two wait-for-endbr states.  One in MSR_U_CET and
one in MSR_S_CET, and the active one is dependent on CPL.

Interrupting CPL3 does leave the state visible (and frozen) in MSR_U_CET.

Interrupting CPL!=0 does not.  The interrupt/exception delivery
microcode forces the wait-for-endbr state which is why the entrypoints
need ENDBR64, and while I haven't confirmed yet, I'm pretty certain that
a WRMSR to MSR_S_CET which sets wait-for-endbr needs an ENDBR64
following it.

IRET does not alter the wait-for-endbr state, but does switch which of
the two trackers is active, as a side effect of changing CPL.


#CP is a decode-class fault, and takes priority over #UD and #NM. 
However, the #BP/#DB special cases are specific to the INT3/INT1
instructions, to specifically permit putting a breakpoint on an ENDBR
instruction and to take the breakpoint exception rather than #CP. 
Critically, #CP has higher priority than General Detect #DB.

(I've already got the beginnings of an XTF test case for shstk corner
cases, but it's got nothing on how complicated the IBT side of things
looks.)

>> Given what a mess it all is, I wouldn't be surprised if the exception
>> delivery microcode has a special case to escalate this to #DF.
> I am meanwhile wondering whether any exception in wait-for-endbr state
> at CPL < 3 would promote to #DF, for loss of state. Albeit there must
> still be a distinction between CALL/JMP induced state and that
> resulting from interrupt or exception delivery. Yet there's no
> architectural (or shadow) state expressing "first insn of an exception
> handler".

Architecturally, no, but there is internally by virtue of the fact that
all interrupt/exception delivery is organised by microcode.

> I'm not even convinced the aforementioned statements about #DB and #BP
> are actually meant to cover more than just CPL3, or at best ENDBR at
> normal CALL/JMP destinations.
>
> While for Xen's own use we may get away without knowing how all of this
> actually works (perhaps accepting the fact that one can't set breakpoints
> at exception handler entry points, depending on whether their delivery
> would promote to #DF), as soon as we were to support CET-IBT for guests
> we'd definitely need to know.

I'm afraid I don't follow the point you're trying to make.  INT3/INT1
are explicitly special, to let you breakpoint an ENDBR instruction, and
that really ought to mean everywhere.

Another possibility is that my observations are an errata in TGL, or
that I've made a mistake somewhere.

However, until we start getting to the point of having some real XTF
tests for behaviour, including that of the emulator, I doubt we'll get
much clarity.  Furthermore, there's a pile of work to do before that is
a possibility.  As a minimum, I need to fix up and repost my xsave
cleanup series, then implement XSAVES support for guests, before we can
make CET an opt-in feature pending completion of the emulation work.

~Andrew

On 06.12.2021 12:38, Andrew Cooper wrote:
> On 06/12/2021 09:42, Jan Beulich wrote:
>> On 03.12.2021 16:30, Andrew Cooper wrote:
>>> On 03/12/2021 13:32, Jan Beulich wrote:
>>>> On 26.11.2021 13:34, Andrew Cooper wrote:
>>>>> Each IDT vector needs to land on an endbr64 instruction.  This is especially
>>>>> important for the #CP handler, which will escalate to #DF if the endbr64 is
>>>>> missing.
>>>> One question here: How does this work?
>>> Honestly, I'm not sure.
>>>
>>>>  I don't recall there being any "CET
>>>> shadow" along the lines of "STI shadow" and "SS shadow", yet there's
>>>> clearly an insn boundary here that gets "skipped" if the 2nd #CP gets
>>>> converted to #DF. And fetching of the first handler insn also isn't part
>>>> of exception delivery (and could cause other exceptions first, like #PF).
>>> I can't make my observations of real hardware behaviour match the
>>> description in the spec.
>> I haven't been able to find a description at all of exception behavior
>> when the exception occurs in wait-for-endbr state. There is text saying
>> that #BP and #DB can occur this way, but I couldn't find anything about
>> the tracker state changes in such cases. While I could see the state to
>> remain engaged (requiring an ENDBR at the handler's entry point), I
>> cannot see how the state would get re-engaged upon IRET from the
>> exception handler, unless the return is back to CPL3.
> 
> Critically, there are two wait-for-endbr states.  One in MSR_U_CET and
> one in MSR_S_CET, and the active one is dependent on CPL.
> 
> Interrupting CPL3 does leave the state visible (and frozen) in MSR_U_CET.
> 
> Interrupting CPL!=0 does not.  The interrupt/exception delivery
> microcode forces the wait-for-endbr state which is why the entrypoints
> need ENDBR64, and while I haven't confirmed yet, I'm pretty certain that
> a WRMSR to MSR_S_CET which sets wait-for-endbr needs an ENDBR64
> following it.

That's my expectation, too.

> IRET does not alter the wait-for-endbr state, but does switch which of
> the two trackers is active, as a side effect of changing CPL.

Well, it only really _switches_ when going from CPL<3 to CPL3. Otherwise
it leaves what is there, meaning wait-for-endbr disengaged. And hence
an interruption at the target of an indirect CALL/JMP will squash the
requirement for an ENDBR to be there. Perhaps a weakness that the
architects considered acceptable and "better" than having to add a way
to save/restore state (where at least the restore part would necessarily
need to be part of IRET itself).

> #CP is a decode-class fault, and takes priority over #UD and #NM. 
> However, the #BP/#DB special cases are specific to the INT3/INT1
> instructions, to specifically permit putting a breakpoint on an ENDBR
> instruction and to take the breakpoint exception rather than #CP. 
> Critically, #CP has higher priority than General Detect #DB.

That's what the text in the spec suggests. Yet why would #DB / #BP be
any more special / important than, say, #PF?

>>> Given what a mess it all is, I wouldn't be surprised if the exception
>>> delivery microcode has a special case to escalate this to #DF.
>> I am meanwhile wondering whether any exception in wait-for-endbr state
>> at CPL < 3 would promote to #DF, for loss of state. Albeit there must
>> still be a distinction between CALL/JMP induced state and that
>> resulting from interrupt or exception delivery. Yet there's no
>> architectural (or shadow) state expressing "first insn of an exception
>> handler".
> 
> Architecturally, no, but there is internally by virtue of the fact that
> all interrupt/exception delivery is organised by microcode.
> 
>> I'm not even convinced the aforementioned statements about #DB and #BP
>> are actually meant to cover more than just CPL3, or at best ENDBR at
>> normal CALL/JMP destinations.
>>
>> While for Xen's own use we may get away without knowing how all of this
>> actually works (perhaps accepting the fact that one can't set breakpoints
>> at exception handler entry points, depending on whether their delivery
>> would promote to #DF), as soon as we were to support CET-IBT for guests
>> we'd definitely need to know.
> 
> I'm afraid I don't follow the point you're trying to make.  INT3/INT1
> are explicitly special, to let you breakpoint an ENDBR instruction, and
> that really ought to mean everywhere.

Maybe. The point I was trying to make was the more general case of an
exception of the ENDBR at the beginning of another exception handler.
Perhaps there indeed is merely a special case for #CP. But that wouldn't
help with e.g. #PF occurring on the #CP handler entry point and the #PF
handler lacking ENDBR. How these cases are meant to work (and which ones
have special treatment) is necessary for us to know the latest when we
want to allow guests to use the features. There, besides getting things
to function correctly, it would also be relevant to know to be sure
there's no XSA-156 equivalent lurking.

Jan