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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 14.02.2022 17:03, Andrew Cooper wrote:
> 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.

Well, yes, it's intentionally the inverted encoding, but I thought
you would get the point.

>>>>> --- 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.

Well, looking at the linker script again I notice that while r/o items
like .init.setup and .initcall*.init come first, some further ones
(.init_array etc) come quite late. Personally I'd prefer if all r/o
items sat side by side, no matter that currently we munge them all
into a single section. Then, if we decided to stop this practice, all
it would take would be to insert an output section closing and re-
opening. (Or it would have been so until now; with your addition it
wouldn't be as simple anymore anyway.)

But anyway, if at this point I still didn't get my point across, then
please leave things as you have them.

Jan