[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH 2/4] x86/altcall: Optimise away endbr64 instruction where possible
On 01/12/2021 08:20, Jan Beulich wrote: > On 26.11.2021 22:22, Andrew Cooper wrote: >> With altcall, we convert indirect branches into direct ones. With that >> complete, none of the potential targets need an endbr64 instruction. > Assuming that no other hooks remain which re-use the same function. I > think this constraint wants at least mentioning explicitly. Fair point, but I think it is entirely reasonable to expect logic not to mix and match altcall on the same hook. > >> Furthermore, removing the endbr64 instructions is a security defence-in-depth >> improvement, because it limits the options available to an attacker who has >> managed to hijack a function pointer. >> >> Introduce a new .init.data.cf_clobber section. Have _apply_alternatives() >> walk over the entire section, looking for any pointers into .text, and >> clobber >> an endbr64 instruction if found. This is some minor structure (ab)use but it >> works alarmingly well. > Iirc you've said more than once that non-function-pointer data in > those structures is fine; I'm not convinced. What if a sequence of > sub-pointer-size fields has a value looking like a pointer into > .text? This may not be very likely, but would result in corruption > that may be hard to associate with anything. Of course, with the > is_endbr64() check and with a build time check of there not being > any stray ENDBR64 patterns in .text, that issue would disappear. > But we aren't quite there yet. I disagree with "not very likely" and put it firmly in the "not plausible" category. To cause a problem, you need an aligned something which isn't actually a function pointer with a bit pattern forming [0xffff82d040200000, ffff82d04039e1ba) which hits an ENDBR64 pattern. Removing the stray ENDBR64's doesn't prevent such a bit pattern pointing at a real (wrong) function. These structures are almost exclusively compile time generated. So yes - it's not impossible, but it's also not going to happen accidentally. > >> --- a/xen/arch/x86/alternative.c >> +++ b/xen/arch/x86/alternative.c >> @@ -173,6 +173,9 @@ text_poke(void *addr, const void *opcode, size_t len) >> return memcpy(addr, opcode, len); >> } >> >> +extern unsigned long __initdata_cf_clobber_start[]; >> +extern unsigned long __initdata_cf_clobber_end[]; > const please. I also would find it quite a bit better if these > were suitably typed such that ... > >> @@ -329,6 +332,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 optimised >> + * all indirect branches to direct ones. >> + */ >> + if ( force && cpu_has_xen_ibt ) >> + { >> + unsigned long *val; >> + unsigned int clobbered = 0; >> + >> + /* >> + * This is some minor structure (ab)use. We walk the entire >> contents >> + * of .init.data.cf_clobber as if it were an array of pointers. >> + * >> + * If the pointer points into .text, and has 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 = (void *)*val; > ... no cast was needed here. Unless you know what this type is, I already tried and am stuck. Everything else requires more horrible casts on val. > >> --- a/xen/arch/x86/xen.lds.S >> +++ b/xen/arch/x86/xen.lds.S >> @@ -214,6 +214,11 @@ SECTIONS >> *(.initcall1.init) >> __initcall_end = .; >> >> + . = ALIGN(POINTER_ALIGN); >> + __initdata_cf_clobber_start = .; >> + *(.init.data.cf_clobber) > Nit: hard tab slipped in here. Will fix. > >> --- a/xen/include/xen/init.h >> +++ b/xen/include/xen/init.h >> @@ -18,6 +18,8 @@ >> #define __init_call(lvl) __used_section(".initcall" lvl ".init") >> #define __exit_call __used_section(".exitcall.exit") >> >> +#define __initdata_cf_clobber __section(".init.data.cf_clobber") > Just to repeat what I've said elsewhere: I think we want a const > version of this as well. I can, but does it really matter? initconst is merged into initdata and not actually read-only to begin with. ~Andrew
|
![]() |
Lists.xenproject.org is hosted with RackSpace, monitoring our |