Re: [PATCH v2 3/7] x86/altcall: Optimise away endbr64 instruction where possible

[Andrew Cooper <Andrew.Cooper3@xxxxxxxxxx>]

On 14/02/2022 13:06, Jan Beulich wrote:
> On 14.02.2022 13:56, Andrew Cooper wrote:
>> With altcall, we convert indirect branches into direct ones.  With that
>> complete, none of the potential targets need an endbr64 instruction.
>>
>> Furthermore, removing the endbr64 instructions is a security defence-in-depth
>> improvement, because it limits the options available to an attacker who has
>> managed to hijack a function pointer.
>>
>> Introduce new .init.{ro,}data.cf_clobber sections.  Have 
>> _apply_alternatives()
>> walk over this, looking for any pointers into .text, and clobber an endbr64
>> instruction if found.  This is some minor structure (ab)use but it works
>> alarmingly well.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>

Thanks,

> with two remarks, which ideally would be addressed by respective
> small adjustments:
>
>> @@ -330,6 +333,41 @@ static void init_or_livepatch 
>> _apply_alternatives(struct alt_instr *start,
>>          add_nops(buf + a->repl_len, total_len - a->repl_len);
>>          text_poke(orig, buf, total_len);
>>      }
>> +
>> +    /*
>> +     * Clobber endbr64 instructions now that altcall has finished optimising
>> +     * all indirect branches to direct ones.
>> +     */
>> +    if ( force && cpu_has_xen_ibt )
>> +    {
>> +        void *const *val;
>> +        unsigned int clobbered = 0;
>> +
>> +        /*
>> +         * This is some minor structure (ab)use.  We walk the entire 
>> contents
>> +         * of .init.{ro,}data.cf_clobber as if it were an array of pointers.
>> +         *
>> +         * If the pointer points into .text, and at an endbr64 instruction,
>> +         * nop out the endbr64.  This causes the pointer to no longer be a
>> +         * legal indirect branch target under CET-IBT.  This is a
>> +         * defence-in-depth measure, to reduce the options available to an
>> +         * adversary who has managed to hijack a function pointer.
>> +         */
>> +        for ( val = __initdata_cf_clobber_start;
>> +              val < __initdata_cf_clobber_end;
>> +              val++ )
>> +        {
>> +            void *ptr = *val;
>> +
>> +            if ( !is_kernel_text(ptr) || !is_endbr64(ptr) )
>> +                continue;
>> +
>> +            add_nops(ptr, 4);
> This literal 4 would be nice to have a #define next to where the ENDBR64
> encoding has its central place.

We don't have an encoding of ENDBR64 in a central place.

The best you can probably have is

#define ENDBR64_LEN 4

in endbr.h ?

>
>> --- a/xen/arch/x86/xen.lds.S
>> +++ b/xen/arch/x86/xen.lds.S
>> @@ -221,6 +221,12 @@ SECTIONS
>>         *(.initcall1.init)
>>         __initcall_end = .;
>>  
>> +       . = ALIGN(POINTER_ALIGN);
>> +       __initdata_cf_clobber_start = .;
>> +       *(.init.data.cf_clobber)
>> +       *(.init.rodata.cf_clobber)
>> +       __initdata_cf_clobber_end = .;
>> +
>>         *(.init.data)
>>         *(.init.data.rel)
>>         *(.init.data.rel.*)
> With r/o data ahead and r/w data following, may I suggest to flip the
> order of the two section specifiers you add?

I don't follow.  This is all initdata which is merged together into a
single section.

The only reason const data is split out in the first place is to appease
the toolchains, not because it makes a difference.

~Andrew