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