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
