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