Re: [PATCH v1.1 64/65] x86/efi: Disable CET-IBT around Runtime Services calls

[Jan Beulich <jbeulich@xxxxxxxx>]

On 10.12.2021 18:16, Andrew Cooper wrote:
> On 06/12/2021 11:06, Jan Beulich wrote:
>> On 26.11.2021 17:38, Andrew Cooper wrote:
>>> --- a/xen/arch/x86/efi/stub.c
>>> +++ b/xen/arch/x86/efi/stub.c
>>> @@ -11,6 +11,8 @@
>>>  #include <efi/efidevp.h>
>>>  #include <efi/efiapi.h>
>>>  
>>> +bool __initdata efi_no_cet_ibt;
>> I'm having trouble seeing what this is needed for - when this file gets
>> built, neither boot.c nor runtime.c will get compiled, and hence there
>> should not be any reference to the symbol that needs satisfying.
>>
>>> @@ -735,6 +736,14 @@ static void __init efi_init(EFI_HANDLE ImageHandle, 
>>> EFI_SYSTEM_TABLE *SystemTabl
>>>  
>>>      StdOut = SystemTable->ConOut;
>>>      StdErr = SystemTable->StdErr ?: StdOut;
>>> +
>>> +#ifdef CONFIG_X86
>> CONFIG_XEN_IBT?
>>
>>> +    /*
>>> +     * Heuristic.  Look under an arbitrary function pointer to see if UEFI 
>>> was
>>> +     * compiled with CET-IBT support.  Experimentally some are not.
>>> +     */
>>> +    efi_no_cet_ibt = !is_endbr64(efi_rs->GetTime);
>> I'm afraid I consider this insufficient. Even if the core EFI was built
>> with IBT support, some driver may not have been.
> 
> That's not an issue.  Everything is built together in practice.

I'd be willing to take your word on this for everything that comes right
with the firmware. I'd further be willing to accept that there are no
add-in card BIOSes which may get involved. But I highly doubt that what
you say applies to all software which may get loaded ahead of starting
Xen. Such software may very well register hooks with core EFI.

>>  Hence I think there
>> needs to be a command line control to force turning off IBT. The only
>> question is whether we want to also honor its positive form - that
>> would, afaict, be a recipe for a guaranteed crash if used wrongly (and
>> it would be meaningless when used on IBT-aware firmware).
> 
> It turns out that IBT support is lacking from tianocore, so nothing is
> going to support IBT for a good while yet.
> 
> https://bugzilla.tianocore.org/show_bug.cgi?id=3726 is the proposed
> change to the spec to support this.
> 
> In the meantime, I'm just going to blanket disable IBT for RS calls.

Yeah, that's going to be okay for the time being.

Jan