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 same logic can check for the absence of
any endbr32 instructions, so include a check for those too.

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, while the behaviour of \10 and
higher depend on the number of capture groups.  Switch the `grep -P` runes to
use hex escapes instead, which are unambiguous

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
v3:
 * Tweak wording about perl regex
 * Check for endbr32 as well
---
 xen/arch/x86/alternative.c       |  2 +-
 xen/arch/x86/include/asm/endbr.h | 26 ++++++++++++++++++++++++++
 xen/tools/check-endbr.sh         | 18 +++++++++++++-----
 3 files changed, 40 insertions(+), 6 deletions(-)

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..641a2342199e 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 all endbr64, endbr32 and nop 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 has various ambiguities with octal-like
+#    escapes, so use hex escapes instead which are unambiguous.
 #
 # 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
@@ -65,11 +67,17 @@ eval $(${OBJDUMP} -j .text $1 -h |
     awk '$2 == ".text" {printf "vma_hi=%s\nvma_lo=%s\n", substr($4, 1, 8), substr($4, 9, 16)}')
 
 ${OBJCOPY} -j .text $1 -O binary $TEXT_BIN
+
+# instruction:    hex:           oct:
+# endbr64         f3 0f 1e fa    363 017 036 372
+# endbr32         f3 0f 1e fb    363 017 036 373
+# nopw (%rcx)     66 0f 1f 01    146 017 037 001
 if $perl_re
 then
-    LC_ALL=C grep -aobP '\363\17\36\372' $TEXT_BIN
+    LC_ALL=C grep -aobP '\xf3\x0f\x1e(\xfa|\xfb)|\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 '\363\17\36\373')" \
+         -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 +98,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} endb32, nop poison, or embedded endbr64 instructions" >&2
 ${ADDR2LINE} -afip -e $1 < $BAD >&2
 exit 1
-- 
2.11.0

On 17.03.2022 15:06, 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 same logic can check for the absence of
> any endbr32 instructions, so include a check for those too.
> 
> 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, while the behaviour of \10 and
> higher depend on the number of capture groups.  Switch the `grep -P` runes to
> use hex escapes instead, which are unambiguous
> 
> 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>
with one nit (which likely I should have spotted before):

> @@ -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 all endbr64, endbr32 and nop 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 has various ambiguities with octal-like
> +#    escapes, so use hex escapes instead which are unambiguous.

There looks to be a stray "can" in here.

Jan

On 17/03/2022 14:21, Jan Beulich wrote:
> On 17.03.2022 15:06, 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 same logic can check for the absence of
>> any endbr32 instructions, so include a check for those too.
>>
>> 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, while the behaviour of \10 and
>> higher depend on the number of capture groups.  Switch the `grep -P` runes to
>> use hex escapes instead, which are unambiguous
>>
>> 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>

Thanks.

> with one nit (which likely I should have spotted before):

Unlikely, seeing as that was part that I rewrote between v2 and v3.

Will fix.

~Andrew