| Message ID | 20220308140126.8815-1-andrew.cooper3@citrix.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | x86/cet: Use dedicated NOP4 for cf_clobber | expand |
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? I'm worried that the pattern you picked is still too close to what might be used (down the road) by a tool chain. If ENDBR32 isn't suitable for some reason, how about "nop %sp" or "nopw (%rsp)" (and then maybe even "data16" substituted by rex, cs, ds, es, or ss)? One neat thing about ENDBR32 would be that you wouldn't even need memcpy() - you'd merely flip the low main opcode bit. > 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? > --- 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. Jan
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
On 08.03.2022 16:19, Andrew Cooper wrote: > 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. Well - I thought of ENDBR32 because elsewhere you said we don't need to be worried about any byte sequence resembling that insn, since for it to become "usable" an attacker would first need to have managed to manufacture a 32-bit ring0 code segment. Now you say effectively the opposite. >> 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. Well, okay - let's hope for the best then. >>> 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. Sounds like exactly what I was thinking of. Perhaps my description wasn't sufficiently clear / unambiguous then. >>> --- 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. Reviewed-by: Jan Beulich <jbeulich@suse.com> Jan
On 08/03/2022 15:36, Jan Beulich wrote: > On 08.03.2022 16:19, Andrew Cooper wrote: >> 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. > Well - I thought of ENDBR32 because elsewhere you said we don't > need to be worried about any byte sequence resembling that insn, > since for it to become "usable" an attacker would first need to > have managed to manufacture a 32-bit ring0 code segment. Now you > say effectively the opposite. We've got ~0 risk of having any embedded ENDBR32's because we never refer to the opcode, and therefore adding 2x 0.7s delay to scan the binary on build is a waste. If the check were free, it would be a different matter. At any point, if we were to introduce references to ENDBR32, we'd want to start scanning for embedded sequences. But at no point do we want to intentionally remove our defence in depth created by both having no CS32 code segment, and no ENDBR32 instructions. > >>> 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. > Well, okay - let's hope for the best then. > >>>> 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. > Sounds like exactly what I was thinking of. Perhaps my description > wasn't sufficiently clear / unambiguous then. Ah yes - I think I did misinterpret what you wrote. I hope everything is clear now. > >>>> --- 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. > Reviewed-by: Jan Beulich <jbeulich@suse.com> Thanks. ~Andrew
On 08.03.22 15:01, Andrew Cooper wrote: > CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you can confirm the sender and know the content is safe. > > > > 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. > > Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> > --- > CC: Jan Beulich <JBeulich@suse.com> > CC: Roger Pau Monné <roger.pau@citrix.com> > CC: Wei Liu <wl@xen.org> > CC: Bjoern Doebel <doebel@amazon.de> > CC: Michael Kurth <mku@amazon.de> > CC: Martin Pohlack <mpohlack@amazon.de> > > Bjoern: For the livepatching code, I think you want: > > if ( is_endbr64(...) || was_endbr64(...) ) > needed += ENDBR64_LEN; > --- > xen/arch/x86/alternative.c | 10 +++++++++- > xen/arch/x86/include/asm/endbr.h | 12 ++++++++++++ > 2 files changed, 21 insertions(+), 1 deletion(-) > > diff --git a/xen/arch/x86/alternative.c b/xen/arch/x86/alternative.c > index d41eeef1bcaf..ffb1b1d960c8 100644 > --- a/xen/arch/x86/alternative.c > +++ b/xen/arch/x86/alternative.c > @@ -362,7 +362,15 @@ static void init_or_livepatch _apply_alternatives(struct alt_instr *start, > if ( !is_kernel_text(ptr) || !is_endbr64(ptr) ) > continue; > > - add_nops(ptr, ENDBR64_LEN); > + /* > + * Can't use add_nops() here. ENDBR64_POISON is specifically > + * different to NOP4 so it can be spotted after the fact. > + * > + * All CET-capable hardware uses P6 NOPS (no need to plumb through > + * ideal_nops), and doesn't require a branch to synchronise the > + * instruction stream. > + */ > + memcpy(ptr, ENDBR64_POISON, ENDBR64_LEN); > clobbered++; > } > > diff --git a/xen/arch/x86/include/asm/endbr.h b/xen/arch/x86/include/asm/endbr.h > index 6090afeb0bd8..5e1e55cb467d 100644 > --- 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) */ > + > +static inline bool was_endbr64(const void *ptr) > +{ > + return *(const uint32_t *)ptr == 0x001f0f66; > +} > + > #endif /* XEN_ASM_ENDBR_H */ > -- > 2.11.0 Reviewed-by: Bjoern Doebel <doebel@amazon.de> Amazon Development Center Germany GmbH Krausenstr. 38 10117 Berlin Geschaeftsfuehrung: Christian Schlaeger, Jonathan Weiss Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B Sitz: Berlin Ust-ID: DE 289 237 879
On 08/03/2022 16:03, Andrew Cooper wrote: >>>>> --- 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. >> Reviewed-by: Jan Beulich <jbeulich@suse.com> > Thanks. It does occur to me that we can extend check-endbr.sh for this. diff --git a/xen/arch/x86/indirect-thunk.S b/xen/arch/x86/indirect-thunk.S index 7cc22da0ef93..3baaf7ab4983 100644 --- a/xen/arch/x86/indirect-thunk.S +++ b/xen/arch/x86/indirect-thunk.S @@ -38,6 +38,7 @@ .section .text.__x86_indirect_thunk_\reg, "ax", @progbits ENTRY(__x86_indirect_thunk_\reg) + nopw (%rax) ALTERNATIVE_2 __stringify(IND_THUNK_RETPOLINE \reg), \ __stringify(IND_THUNK_LFENCE \reg), X86_FEATURE_IND_THUNK_LFENCE, \ __stringify(IND_THUNK_JMP \reg), X86_FEATURE_IND_THUNK_JMP diff --git a/xen/tools/check-endbr.sh b/xen/tools/check-endbr.sh index 9799c451a18d..652ac8d0b983 100755 --- a/xen/tools/check-endbr.sh +++ b/xen/tools/check-endbr.sh @@ -67,7 +67,7 @@ eval $(${OBJDUMP} -j .text $1 -h | ${OBJCOPY} -j .text $1 -O binary $TEXT_BIN if $perl_re then - LC_ALL=C grep -aobP '\363\17\36\372' $TEXT_BIN + LC_ALL=C grep -aobP '\363\17\36\372|\x66\x0f\x1f\x00' $TEXT_BIN else grep -aob "$(printf '\363\17\36\372')" $TEXT_BIN fi | awk -F':' '{printf "%s%x\n", "'$vma_hi'", int(0x'$vma_lo') + $1}' > $ALL yields: check-endbr.sh xen-syms Fail: Found 15 embedded endbr64 instructions 0xffff82d040377f00: __x86_indirect_thunk_rax at /local/xen.git/xen/arch/x86/indirect-thunk.S:55 0xffff82d040377f20: __x86_indirect_thunk_rcx at ??:? 0xffff82d040377f40: __x86_indirect_thunk_rdx at ??:? 0xffff82d040377f60: __x86_indirect_thunk_rbx at ??:? 0xffff82d040377f80: __x86_indirect_thunk_rbp at ??:? 0xffff82d040377fa0: __x86_indirect_thunk_rsi at ??:? 0xffff82d040377fc0: __x86_indirect_thunk_rdi at ??:? 0xffff82d040377fe0: __x86_indirect_thunk_r8 at ??:? 0xffff82d040378000: __x86_indirect_thunk_r9 at ??:? 0xffff82d040378020: __x86_indirect_thunk_r10 at ??:? 0xffff82d040378040: __x86_indirect_thunk_r11 at ??:? 0xffff82d040378060: __x86_indirect_thunk_r12 at ??:? 0xffff82d040378080: __x86_indirect_thunk_r13 at ??:? 0xffff82d0403780a0: __x86_indirect_thunk_r14 at ??:? 0xffff82d0403780c0: __x86_indirect_thunk_r15 at ??:? ... check-endbr.sh xen.efi Fail: Found 15 embedded endbr64 instructions 0xffff82d040377f00: ?? at /local/xen.git/xen/arch/x86/indirect-thunk.S:55 0xffff82d040377f20: ?? at head.o:? 0xffff82d040377f40: ?? at head.o:? 0xffff82d040377f60: ?? at head.o:? 0xffff82d040377f80: ?? at head.o:? 0xffff82d040377fa0: ?? at head.o:? 0xffff82d040377fc0: ?? at head.o:? 0xffff82d040377fe0: ?? at head.o:? 0xffff82d040378000: ?? at head.o:? 0xffff82d040378020: ?? at head.o:? 0xffff82d040378040: ?? at head.o:? 0xffff82d040378060: ?? at head.o:? 0xffff82d040378080: ?? at head.o:? 0xffff82d0403780a0: ?? at head.o:? 0xffff82d0403780c0: ?? at head.o:? Obviously the changes to check-endbr want cleaning up, but I think it's entirely within scope to check for ENDBR64_POISON too, and we can do it without adding an extra pass. Would you be happier with this check added? But we also have some clear errors with debug symbols. It's perhaps not terribly surprising that irp/endr only gets file/line for the first instance, and at least ELF manage to get the function name right, but EFI is a mess and manages to get the wrong file. Any idea how to get rather less nonsense out of the debug symbols? ~Andrew
On 10.03.2022 19:42, Andrew Cooper wrote: > On 08/03/2022 16:03, Andrew Cooper wrote: >>>>>> --- 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. >>> Reviewed-by: Jan Beulich <jbeulich@suse.com> >> Thanks. > > It does occur to me that we can extend check-endbr.sh for this. > > diff --git a/xen/arch/x86/indirect-thunk.S b/xen/arch/x86/indirect-thunk.S > index 7cc22da0ef93..3baaf7ab4983 100644 > --- a/xen/arch/x86/indirect-thunk.S > +++ b/xen/arch/x86/indirect-thunk.S > @@ -38,6 +38,7 @@ > .section .text.__x86_indirect_thunk_\reg, "ax", @progbits > > ENTRY(__x86_indirect_thunk_\reg) > + nopw (%rax) > ALTERNATIVE_2 __stringify(IND_THUNK_RETPOLINE \reg), \ > __stringify(IND_THUNK_LFENCE \reg), X86_FEATURE_IND_THUNK_LFENCE, \ > __stringify(IND_THUNK_JMP \reg), X86_FEATURE_IND_THUNK_JMP > diff --git a/xen/tools/check-endbr.sh b/xen/tools/check-endbr.sh > index 9799c451a18d..652ac8d0b983 100755 > --- a/xen/tools/check-endbr.sh > +++ b/xen/tools/check-endbr.sh > @@ -67,7 +67,7 @@ eval $(${OBJDUMP} -j .text $1 -h | > ${OBJCOPY} -j .text $1 -O binary $TEXT_BIN > if $perl_re > then > - LC_ALL=C grep -aobP '\363\17\36\372' $TEXT_BIN > + LC_ALL=C grep -aobP '\363\17\36\372|\x66\x0f\x1f\x00' $TEXT_BIN > else > grep -aob "$(printf '\363\17\36\372')" $TEXT_BIN > fi | awk -F':' '{printf "%s%x\n", "'$vma_hi'", int(0x'$vma_lo') + $1}' >> $ALL > > yields: > > check-endbr.sh xen-syms Fail: Found 15 embedded endbr64 instructions > 0xffff82d040377f00: __x86_indirect_thunk_rax at > /local/xen.git/xen/arch/x86/indirect-thunk.S:55 > 0xffff82d040377f20: __x86_indirect_thunk_rcx at ??:? > 0xffff82d040377f40: __x86_indirect_thunk_rdx at ??:? > 0xffff82d040377f60: __x86_indirect_thunk_rbx at ??:? > 0xffff82d040377f80: __x86_indirect_thunk_rbp at ??:? > 0xffff82d040377fa0: __x86_indirect_thunk_rsi at ??:? > 0xffff82d040377fc0: __x86_indirect_thunk_rdi at ??:? > 0xffff82d040377fe0: __x86_indirect_thunk_r8 at ??:? > 0xffff82d040378000: __x86_indirect_thunk_r9 at ??:? > 0xffff82d040378020: __x86_indirect_thunk_r10 at ??:? > 0xffff82d040378040: __x86_indirect_thunk_r11 at ??:? > 0xffff82d040378060: __x86_indirect_thunk_r12 at ??:? > 0xffff82d040378080: __x86_indirect_thunk_r13 at ??:? > 0xffff82d0403780a0: __x86_indirect_thunk_r14 at ??:? > 0xffff82d0403780c0: __x86_indirect_thunk_r15 at ??:? > ... > check-endbr.sh xen.efi Fail: Found 15 embedded endbr64 instructions > 0xffff82d040377f00: ?? at /local/xen.git/xen/arch/x86/indirect-thunk.S:55 > 0xffff82d040377f20: ?? at head.o:? > 0xffff82d040377f40: ?? at head.o:? > 0xffff82d040377f60: ?? at head.o:? > 0xffff82d040377f80: ?? at head.o:? > 0xffff82d040377fa0: ?? at head.o:? > 0xffff82d040377fc0: ?? at head.o:? > 0xffff82d040377fe0: ?? at head.o:? > 0xffff82d040378000: ?? at head.o:? > 0xffff82d040378020: ?? at head.o:? > 0xffff82d040378040: ?? at head.o:? > 0xffff82d040378060: ?? at head.o:? > 0xffff82d040378080: ?? at head.o:? > 0xffff82d0403780a0: ?? at head.o:? > 0xffff82d0403780c0: ?? at head.o:? > > Obviously the changes to check-endbr want cleaning up, but I think it's > entirely within scope to check for ENDBR64_POISON too, and we can do it > without adding an extra pass. Would you be happier with this check added? Yes, this would feel better. Thanks for having continued to think about it. > But we also have some clear errors with debug symbols. It's perhaps not > terribly surprising that irp/endr only gets file/line for the first > instance, I have to admit I would expect it to at least figure the file. But there's no .debug_line contents at all for ..._rcx .. ..._r15. > and at least ELF manage to get the function name right, but > EFI is a mess and manages to get the wrong file. Any idea how to get > rather less nonsense out of the debug symbols? A random example with a symbol from a C file works here, at least. Jan
diff --git a/xen/arch/x86/alternative.c b/xen/arch/x86/alternative.c index d41eeef1bcaf..ffb1b1d960c8 100644 --- a/xen/arch/x86/alternative.c +++ b/xen/arch/x86/alternative.c @@ -362,7 +362,15 @@ static void init_or_livepatch _apply_alternatives(struct alt_instr *start, if ( !is_kernel_text(ptr) || !is_endbr64(ptr) ) continue; - add_nops(ptr, ENDBR64_LEN); + /* + * Can't use add_nops() here. ENDBR64_POISON is specifically + * different to NOP4 so it can be spotted after the fact. + * + * All CET-capable hardware uses P6 NOPS (no need to plumb through + * ideal_nops), and doesn't require a branch to synchronise the + * instruction stream. + */ + memcpy(ptr, ENDBR64_POISON, ENDBR64_LEN); clobbered++; } diff --git a/xen/arch/x86/include/asm/endbr.h b/xen/arch/x86/include/asm/endbr.h index 6090afeb0bd8..5e1e55cb467d 100644 --- 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) */ + +static inline bool was_endbr64(const void *ptr) +{ + return *(const uint32_t *)ptr == 0x001f0f66; +} + #endif /* XEN_ASM_ENDBR_H */
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. Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> --- CC: Jan Beulich <JBeulich@suse.com> CC: Roger Pau Monné <roger.pau@citrix.com> CC: Wei Liu <wl@xen.org> CC: Bjoern Doebel <doebel@amazon.de> CC: Michael Kurth <mku@amazon.de> CC: Martin Pohlack <mpohlack@amazon.de> Bjoern: For the livepatching code, I think you want: if ( is_endbr64(...) || was_endbr64(...) ) needed += ENDBR64_LEN; --- xen/arch/x86/alternative.c | 10 +++++++++- xen/arch/x86/include/asm/endbr.h | 12 ++++++++++++ 2 files changed, 21 insertions(+), 1 deletion(-)