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

[Andrew Cooper <Andrew.Cooper3@citrix.com>]

On 14/02/2022 13:51, Jan Beulich wrote:
> On 14.02.2022 14:31, Andrew Cooper wrote:
>> On 14/02/2022 13:06, Jan Beulich wrote:
>>> On 14.02.2022 13:56, Andrew Cooper wrote:
>>>> @@ -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 ?
> Perhaps. That's not in this series nor in staging already, so it's a little
> hard to check. By "central place" I really meant is_enbr64() if that's the
> only place where the encoding actually appears.

endbr.h is the header which contains is_endbr64(), and deliberately does
not contain the raw encoding.

>
>>>> --- 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.
> It's marginal, I agree, but it would still seem more clean to me if all
> (pseudo) r/o init data lived side by side.

I still don't understand what you're asking.

There is no such thing as actually read-only init data.

Wherever the .init.rodata goes in here, it's bounded by .init.data.

~Andrew