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
