UEFI Runtime services, at the time of writing, aren't CET-IBT compatible.
Work is ongoing to address this. In the meantime, unconditionally disable IBT.

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>

https://bugzilla.tianocore.org/show_bug.cgi?id=3726 is the upstream tracking
ticket.

v2:
 * Rewrite to be an unconditional disable.
---
 xen/common/efi/runtime.c | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/xen/common/efi/runtime.c b/xen/common/efi/runtime.c
index e3ce85d118e4..13b0975866e3 100644
--- a/xen/common/efi/runtime.c
+++ b/xen/common/efi/runtime.c
@@ -21,6 +21,7 @@ struct efi_rs_state {
   * don't strictly need that.
   */
  unsigned long __aligned(32) cr3;
+    unsigned long msr_s_cet;
 #endif
 };
 
@@ -113,6 +114,19 @@ struct efi_rs_state efi_rs_enter(void)
 
     switch_cr3_cr4(mfn_to_maddr(efi_l4_mfn), read_cr4());
 
+    /*
+     * At the time of writing (2022), no UEFI firwmare is CET-IBT compatible.
+     * Work is under way to remedy this.
+     *
+     * Stash MSR_S_CET and clobber ENDBR_EN.  This is necessary because
+     * SHSTK_EN isn't configured until very late on the BSP.
+     */
+    if ( cpu_has_xen_ibt )
+    {
+        rdmsrl(MSR_S_CET, state.msr_s_cet);
+        wrmsrl(MSR_S_CET, state.msr_s_cet & ~CET_ENDBR_EN);
+    }
+
     return state;
 }
 
@@ -122,6 +136,10 @@ void efi_rs_leave(struct efi_rs_state *state)
 
     if ( !state->cr3 )
         return;
+
+    if ( state->msr_s_cet )
+        wrmsrl(MSR_S_CET, state->msr_s_cet);
+
     switch_cr3_cr4(state->cr3, read_cr4());
     if ( is_pv_vcpu(curr) && !is_idle_vcpu(curr) )
     {
-- 
2.11.0

On 14.02.2022 13:51, Andrew Cooper wrote:
> UEFI Runtime services, at the time of writing, aren't CET-IBT compatible.
> Work is ongoing to address this. In the meantime, unconditionally disable IBT.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

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

> --- a/xen/common/efi/runtime.c
> +++ b/xen/common/efi/runtime.c
> @@ -21,6 +21,7 @@ struct efi_rs_state {
>    * don't strictly need that.
>    */
>   unsigned long __aligned(32) cr3;
> +    unsigned long msr_s_cet;
>  #endif
>  };

The latest with the next addition here we will probably want to ...

> @@ -113,6 +114,19 @@ struct efi_rs_state efi_rs_enter(void)

... no longer have this be the function's return type.

Jan

On 15/02/2022 16:53, Jan Beulich wrote:
> On 14.02.2022 13:51, Andrew Cooper wrote:
>> UEFI Runtime services, at the time of writing, aren't CET-IBT compatible.
>> Work is ongoing to address this. In the meantime, unconditionally disable IBT.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Reviewed-by: Jan Beulich <jbeulich@suse.com>

Thanks.

>
>> --- a/xen/common/efi/runtime.c
>> +++ b/xen/common/efi/runtime.c
>> @@ -21,6 +21,7 @@ struct efi_rs_state {
>>    * don't strictly need that.
>>    */
>>   unsigned long __aligned(32) cr3;
>> +    unsigned long msr_s_cet;
>>  #endif
>>  };
> The latest with the next addition here we will probably want to ...
>
>> @@ -113,6 +114,19 @@ struct efi_rs_state efi_rs_enter(void)
> ... no longer have this be the function's return type.

So about this.

why aren't we using __attribute__((force_align_arg_pointer)) ?  It
exists in at least GCC 4.1 and Clang 6.

We're way way overdue bumping the minimum toolchain versions, and Clang
3.5=>6 is still very obsolete minimum version.  This way, we're not
depending on some very subtle ABI mechanics to try and keep the stack
properly aligned.

~Andrew

On 16.02.2022 00:00, Andrew Cooper wrote:
> On 15/02/2022 16:53, Jan Beulich wrote:
>> On 14.02.2022 13:51, Andrew Cooper wrote:
>>> --- a/xen/common/efi/runtime.c
>>> +++ b/xen/common/efi/runtime.c
>>> @@ -21,6 +21,7 @@ struct efi_rs_state {
>>>    * don't strictly need that.
>>>    */
>>>   unsigned long __aligned(32) cr3;
>>> +    unsigned long msr_s_cet;
>>>  #endif
>>>  };
>> The latest with the next addition here we will probably want to ...
>>
>>> @@ -113,6 +114,19 @@ struct efi_rs_state efi_rs_enter(void)
>> ... no longer have this be the function's return type.
> 
> So about this.
> 
> why aren't we using __attribute__((force_align_arg_pointer)) ?  It
> exists in at least GCC 4.1 and Clang 6.

Perhaps first and foremost because this is the first time I encounter
this attribute, despite it having been around for so long. However,
Clang 6 would be a little too high for the main box I have a Clang
installed on - that's Clang 5 only (and, afaict, no option to upgrade
without also upgrading the distro, while I'd also like to avoid having
to also build myself Clang binaries; maybe sooner or later that's
going to be unavoidable, though). While from binary searching its
libraries it looks to know of that attribute, it still doesn't accept
its use.

The other issue I see is that using it would be fragile: We cannot
afford to forget putting the attribute on any of the relevant
functions. Whereas the present model makes it impossible to miss
any instance.

Finally the attribute's interaction with -mpreferred-stack-boundary=
isn't spelled out anywhere. It looks to behave sanely on gcc 11, but
who knows whether this has always been the case.

Jan

> We're way way overdue bumping the minimum toolchain versions, and Clang
> 3.5=>6 is still very obsolete minimum version.  This way, we're not
> depending on some very subtle ABI mechanics to try and keep the stack
> properly aligned.
> 
> ~Andrew