[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH v2 3/7] x86/altcall: Optimise away endbr64 instruction where possible
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
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |