| Message ID | 20220317100204.16391-1-andrew.cooper3@citrix.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | x86/cet: Use dedicated NOP4 for cf_clobber | expand |
On 17.03.2022 11:02, 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 extend > check-endbr.sh to look for it. > > The choice of nop has some complicated consequences. nopw (%rax) has a ModRM > byte of 0, which the Bourne compatible shells unconditionally strip from > parameters, meaning that we can't pass it to `grep -aob`. Urgh. But as per my earlier comments I'm happier with ... > Therefore, use nopw (%rcx) so the ModRM byte becomes 1. ... a non-zero ModR/M byte anyway. > This then demonstrates another bug. Under perl regexes, \1 thru \9 are > subpattern matches, and not octal escapes. Switch the `grep -P` runes to use > hex escapes instead. > > The build time check then requires that the endbr64 poison have the same > treatment as endbr64 to avoid placing the byte pattern in immediate operands. > > Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> Reviewed-by: Jan Beulich <jbeulich@suse.com> > Jan: As you had the buggy grep, can you please confirm that hex escapes work. (Build-)Tested-by: Jan Beulich <jbeulich@suse.com> When working out the workaround, I actually did test with hex, but then switched to octal to make easily visible that the two patterns actually match. I also did wonder about octal and sub-pattern matching conflicting, but the grep I used definitely didn't have an issue there. Hence I assume grep behavior changed at some point; I wonder how they mean to have octal expressed now. https://perldoc.perl.org/perlre specifically outlines how the conflict is dealt with - assuming you have observed grep to misbehave, I wonder whether they've accumulated a bug. (The doc also makes clear that such references aren't limited to single digit numbers; you may want to adjust your description in this regard.) Depending on how exactly your grep behaves, switching to always-three- digit octal escapes may be an alternative to retain the property of making obvious the similarity between the two pattern representations. Jan
On 17/03/2022 10:43, Jan Beulich wrote: > On 17.03.2022 11:02, 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 extend >> check-endbr.sh to look for it. >> >> The choice of nop has some complicated consequences. nopw (%rax) has a ModRM >> byte of 0, which the Bourne compatible shells unconditionally strip from >> parameters, meaning that we can't pass it to `grep -aob`. > Urgh. But as per my earlier comments I'm happier with ... > >> Therefore, use nopw (%rcx) so the ModRM byte becomes 1. > ... a non-zero ModR/M byte anyway. > >> This then demonstrates another bug. Under perl regexes, \1 thru \9 are >> subpattern matches, and not octal escapes. Switch the `grep -P` runes to use >> hex escapes instead. >> >> The build time check then requires that the endbr64 poison have the same >> treatment as endbr64 to avoid placing the byte pattern in immediate operands. >> >> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> > Reviewed-by: Jan Beulich <jbeulich@suse.com> > >> Jan: As you had the buggy grep, can you please confirm that hex escapes work. > (Build-)Tested-by: Jan Beulich <jbeulich@suse.com> Thanks. > > When working out the workaround, I actually did test with hex, but > then switched to octal to make easily visible that the two patterns > actually match. I also did wonder about octal and sub-pattern > matching conflicting, but the grep I used definitely didn't have > an issue there. Hence I assume grep behavior changed at some point; > I wonder how they mean to have octal expressed now. $ LC_ALL=C grep -aobP '\363\17\36\372|\146\17\37\1' text.bin grep: reference to non-existent subpattern > https://perldoc.perl.org/perlre specifically outlines how the > conflict is dealt with - assuming you have observed grep to misbehave, > I wonder whether they've accumulated a bug. (The doc also makes clear > that such references aren't limited to single digit numbers; you may > want to adjust your description in this regard.) That part of the doc does say that the dynamic interpretation is only for \10 and higher, so I don't think this is a bug. \1 use to exclusively mean the first capture group, not an octal character, and this behaviour remains. > Depending on how exactly your grep behaves, switching to always-three- > digit octal escapes may be an alternative to retain the property of > making obvious the similarity between the two pattern representations. \01 and \001 do both work properly, but the non-ambiguous forms are \o1 or \o001. Overall, I think it's better to stay with the hex escapes, because they're also non-ambiguous. The mix of octal and hex is irritating, but the comments are very clear about what we're searching for. And on that note, I realise we can also scan for endbr32 in exactly the same way for no extra cost. I'll fold that in too, seeing as the discussion has come up before, and post a v3. ~Andrew
diff --git a/xen/arch/x86/alternative.c b/xen/arch/x86/alternative.c index d41eeef1bcaf..0c6fc7b4fb0c 100644 --- a/xen/arch/x86/alternative.c +++ b/xen/arch/x86/alternative.c @@ -362,7 +362,7 @@ 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); + place_endbr64_poison(ptr); clobbered++; } diff --git a/xen/arch/x86/include/asm/endbr.h b/xen/arch/x86/include/asm/endbr.h index 6090afeb0bd8..d946fac13130 100644 --- a/xen/arch/x86/include/asm/endbr.h +++ b/xen/arch/x86/include/asm/endbr.h @@ -52,4 +52,30 @@ 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. Specifically, nopw (%rcx) + */ +static inline uint32_t __attribute_const__ gen_endbr64_poison(void) +{ + uint32_t res; + + asm ( "mov $~0x011f0f66, %[res]\n\t" + "not %[res]\n\t" + : [res] "=&r" (res) ); + + return res; +} + +static inline bool is_endbr64_poison(const void *ptr) +{ + return *(const uint32_t *)ptr == gen_endbr64_poison(); +} + +static inline void place_endbr64_poison(void *ptr) +{ + *(uint32_t *)ptr = gen_endbr64_poison(); +} + #endif /* XEN_ASM_ENDBR_H */ diff --git a/xen/tools/check-endbr.sh b/xen/tools/check-endbr.sh index 9799c451a18d..126a2a14d44e 100755 --- a/xen/tools/check-endbr.sh +++ b/xen/tools/check-endbr.sh @@ -27,7 +27,7 @@ echo "X" | grep -aob "X" -q 2>/dev/null || # Check whether grep supports Perl regexps. Older GNU grep doesn't reliably # find binary patterns otherwise. perl_re=true -echo "X" | grep -aobP "\130" -q 2>/dev/null || perl_re=false +echo "X" | grep -aobP "\x58" -q 2>/dev/null || perl_re=false # # First, look for all the valid endbr64 instructions. @@ -45,13 +45,15 @@ echo "X" | grep -aobP "\130" -q 2>/dev/null || perl_re=false ${OBJDUMP} -j .text $1 -d -w | grep ' endbr64 *$' | cut -f 1 -d ':' > $VALID & # -# Second, look for any endbr64 byte sequence +# Second, look for any endbr64 or nop4 poison byte sequences # This has a couple of complications: # # 1) Grep binary search isn't VMA aware. Copy .text out as binary, causing # the grep offset to be from the start of .text. # # 2) dash's printf doesn't understand hex escapes, hence the use of octal. +# `grep -P` on the other hand can interpret hex escapes, and must use them +# to avoid \1 thru \9 being interpreted as subpatterns matches. # # 3) AWK can't add 64bit integers, because internally all numbers are doubles. # When the upper bits are set, the exponents worth of precision is lost in @@ -67,9 +69,9 @@ 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 '\xf3\x0f\x1e\xfa|\x66\x0f\x1f\x01' $TEXT_BIN else - grep -aob "$(printf '\363\17\36\372')" $TEXT_BIN + grep -aob -e "$(printf '\363\17\36\372')" -e "$(printf '\146\17\37\1')" $TEXT_BIN fi | awk -F':' '{printf "%s%x\n", "'$vma_hi'", int(0x'$vma_lo') + $1}' > $ALL # Wait for $VALID to become complete @@ -90,6 +92,6 @@ nr_bad=$(wc -l < $BAD) [ "$nr_bad" -eq 0 ] && exit 0 # Failure -echo "$MSG_PFX Fail: Found ${nr_bad} embedded endbr64 instructions" >&2 +echo "$MSG_PFX Fail: Found ${nr_bad} embedded endbr64 or poison instructions" >&2 ${ADDR2LINE} -afip -e $1 < $BAD >&2 exit 1
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 extend check-endbr.sh to look for it. The choice of nop has some complicated consequences. nopw (%rax) has a ModRM byte of 0, which the Bourne compatible shells unconditionally strip from parameters, meaning that we can't pass it to `grep -aob`. Therefore, use nopw (%rcx) so the ModRM byte becomes 1. This then demonstrates another bug. Under perl regexes, \1 thru \9 are subpattern matches, and not octal escapes. Switch the `grep -P` runes to use hex escapes instead. The build time check then requires that the endbr64 poison have the same treatment as endbr64 to avoid placing the byte pattern in immediate operands. 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> v2: * Check for the poison byte pattern in check-endbr.sh * Use nopw (%rcx) to work around shell NUL (mis)features * Use hex escapes to work around Perl subpattern matches Jan: As you had the buggy grep, can you please confirm that hex escapes work. --- xen/arch/x86/alternative.c | 2 +- xen/arch/x86/include/asm/endbr.h | 26 ++++++++++++++++++++++++++ xen/tools/check-endbr.sh | 12 +++++++----- 3 files changed, 34 insertions(+), 6 deletions(-)