Re: [PATCH 5/5] x86/HVM: improve CET-IBT pruning of ENDBR

[Jan Beulich <jbeulich@xxxxxxxx>]

On 22.11.2023 11:08, Roger Pau Monné wrote:
> On Thu, Nov 16, 2023 at 02:33:14PM +0100, Jan Beulich wrote:
>> --- a/xen/arch/x86/hvm/svm/svm.c
>> +++ b/xen/arch/x86/hvm/svm/svm.c
>> @@ -2587,6 +2587,19 @@ const struct hvm_function_table * __init
>>      return &svm_function_table;
>>  }
>>  
>> +void __init prune_svm(void)
>> +{
>> +    /*
>> +     * Now that svm_function_table was copied, populate all function 
>> pointers
>> +     * which may have been left at NULL, for __initdata_cf_clobber to have 
>> as
>> +     * much of an effect as possible.
>> +     */
>> +    if ( !IS_ENABLED(CONFIG_XEN_IBT) )
> 
> Shouldn't this better use cpu_has_xen_ibt?
> 
> Otherwise the clobbering done in _apply_alternatives() won't be
> engaged, so it's pointless to set the extra fields.

That's better answered in the context of ...

>> --- a/xen/arch/x86/hvm/vmx/vmx.c
>> +++ b/xen/arch/x86/hvm/vmx/vmx.c
>> @@ -3032,6 +3032,30 @@ const struct hvm_function_table * __init
>>      return &vmx_function_table;
>>  }
>>  
>> +void __init prune_vmx(void)
>> +{
>> +    /*
>> +     * Now that vmx_function_table was copied, populate all function 
>> pointers
>> +     * which may have been left at NULL, for __initdata_cf_clobber to have 
>> as
>> +     * much of an effect as possible.
>> +     */
>> +    if ( !IS_ENABLED(CONFIG_XEN_IBT) )
>> +        return;
>> +
>> +    vmx_function_table.set_descriptor_access_exiting =
>> +        vmx_set_descriptor_access_exiting;
>> +
>> +    vmx_function_table.update_eoi_exit_bitmap = vmx_update_eoi_exit_bitmap;
>> +    vmx_function_table.process_isr            = vmx_process_isr;
>> +    vmx_function_table.handle_eoi             = vmx_handle_eoi;
>> +
>> +    vmx_function_table.pi_update_irte = vmx_pi_update_irte;
>> +
>> +    vmx_function_table.deliver_posted_intr = vmx_deliver_posted_intr;
>> +    vmx_function_table.sync_pir_to_irr     = vmx_sync_pir_to_irr;
>> +    vmx_function_table.test_pir            = vmx_test_pir;

... this: The goal of having a compile time conditional was to have the
compiler eliminate the code when not needed. Otherwise there's no real
reason to have a conditional there in the first place - we can as well
always install all these pointers.

> Hm, I find this quite fragile, as it's easy to add a new handler
> without realizing that addition here might also be required.

Indeed, but that's not the end of the world (as much as so far it
wasn't deemed necessary at all to try and also purge unused hooks'
ENDBR).

> I don't really have good ideas about how to handle this, unless we
> populate unused handlers with some poison and loop over the structure
> as an array of pointers and choke on finding one of them pointing to
> NULL.

The looping over the resulting section is already somewhat fragile,
when considering other non-pointer data in there. A specific poison
value would further increase the risk of mistaking a value for what
it doesn't represent, even if just a tiny bit.

Populating unused handlers with some poison value would also have the
same problem of being easy to forget when adding a new hook.

Jan