On 08/03/2022 14:37, Jan Beulich wrote: > On 08.03.2022 15:01, Andrew Cooper wrote: >> For livepatching, we need to look at a potentially clobbered function and >> determine whether it used to have an ENDBR64 instruction. >> >> Use a non-default 4-byte P6 long nop, not emitted by toolchains, and >> introduce >> the was_endbr64() predicate. > Did you consider using ENDBR32 for this purpose?
No, and no because that's very short sighted. Even 4 non-nops would be better than ENDBR32, because they wouldn't create actually-usable codepaths in corner cases we care to exclude. > I'm worried that > the pattern you picked is still too close to what might be used > (down the road) by a tool chain. This is what Linux are doing too, but no - I'm not worried. The encoding isn't the only protection; toolchains also have no reason to put a nop4 at the head of functions; nop5 is the common one to find. > One neat thing about ENDBR32 would be that you wouldn't even > need memcpy() - you'd merely flip the low main opcode bit. Not relevant. You're taking the SMC pipeline hit for any sized write, and a single movl is far less cryptic. >> Bjoern: For the livepatching code, I think you want: >> >> if ( is_endbr64(...) || was_endbr64(...) ) >> needed += ENDBR64_LEN; > Looks like I didn't fully understand the problem then from your > initial description. The adjustment here (and the one needed in > Björn's patch) is to compensate for the advancing of the > targets of altcalls past the ENDBR? No. Consider this scenario: callee: endbr64 ... altcall: call *foo(%rip) During boot, we rewrite altcall to be `call callee+4` and turn endbr64 into nops, so it now looks like: callee: nop4 ... altcall: call callee+4 Then we want to livepatch callee to callee_new, so we get callee_new: endbr64 ... in the livepatch itself. Now, to actually patch, we need to memcpy(callee+4, "jmp callee_new", 5). The livepatch logic calling is_endbr(callee) doesn't work because it's now a nop4, which is why it needs a was_endbr64(callee) too. > >> --- a/xen/arch/x86/include/asm/endbr.h >> +++ b/xen/arch/x86/include/asm/endbr.h >> @@ -52,4 +52,16 @@ static inline void place_endbr64(void *ptr) >> *(uint32_t *)ptr = gen_endbr64(); >> } >> >> +/* >> + * After clobbering ENDBR64, we may need to confirm that the site used to >> + * contain an ENDBR64 instruction. Use an encoding which isn't the default >> + * P6_NOP4. >> + */ >> +#define ENDBR64_POISON "\x66\x0f\x1f\x00" /* osp nopl (%rax) */ > In case this remains as is - did you mean "opsz" instead of "osp"? > But this really is "nopw (%rax)" anyway. Oh, osp is the nasm name. I'll switch to nopw. ~Andrew
