On 23/02/2022 11:31, Jan Beulich wrote:
> On 22.02.2022 16:26, Andrew Cooper wrote:
>> up on a non-instruction boundary. Such embedded instructions mark legal
>> indirect branch targets as far as the CPU is concerned, which aren't legal as
>> far as the logic is concerned.
> Thinking about it: Wouldn't it be yet slightly more reassuring to also
> look for ENDBR32?
I considered that, but it's awkward to do and doubles the length of this
already ~0.7s (x2 for efi because this step isn't performed in parallel)
delay to the build.
We do not have __HYPERVISOR_CS32, so ENDBR32 will yield #CP[endbr] if
encountered.
If an attacker has managed to edit the GDT to insert a compatibility
code segment, and hijacked a far transfer to use it, then the absence of
ENDBR32's in the binary isn't going to be an impediment.
>
>> When CET-IBT is active, check for embedded byte sequences. Example failures
>> look like:
>>
>> check-endbr.sh xen-syms Fail: Found 2 embedded endbr64 instructions
>> 0xffff82d040325677: test_endbr64 at
>> /local/xen.git/xen/arch/x86/x86_64/entry.S:28
>> 0xffff82d040352da6: init_done at /local/xen.git/xen/arch/x86/setup.c:675
>>
>> Signed-off-by: Marek Marczykowski-Górecki <marma...@invisiblethingslab.com>
>> Signed-off-by: Andrew Cooper <andrew.coop...@citrix.com>
> Reviewed-by: Jan Beulich <jbeul...@suse.com>
Thanks.
>
>> --- a/README
>> +++ b/README
>> @@ -68,6 +68,7 @@ provided by your OS distributor:
>> In addition to the above there are a number of optional build
>> prerequisites. Omitting these will cause the related features to be
>> disabled at compile time:
>> + * Binary-search capable grep (if building Xen with CET support)
> Nit: With this (maybe this was the case already earlier though)
> s/will/may/ in the previous sentence?
I'm planning a separate overhaul to README because bits of it are quite
wrong, including lots of this section. This was the lead bad addition I
could come up with that didn't involve a major rewrite.
>> --- /dev/null
>> +++ b/xen/tools/check-endbr.sh
>> @@ -0,0 +1,85 @@
>> +#!/bin/sh
>> +#
>> +# Usage ./$0 xen-syms
>> +#
>> +set -e
>> +
>> +# Prettyprint parameters a little for message
>> +MSG_PFX="${0##*/} ${1##*/}"
>> +
>> +OBJCOPY="${OBJCOPY:-objcopy} -j .text $1"
>> +OBJDUMP="${OBJDUMP:-objdump} -j .text $1"
> While embedding the arguments here shortens the lines where these are
> used, the appearance especially of $OBJCOPY with a single file name
> argument ...
>
>> +ADDR2LINE="${ADDR2LINE:-addr2line}"
>> +
>> +D=$(mktemp -d)
>> +trap "rm -rf $D" EXIT
>> +
>> +TEXT_BIN=$D/xen-syms.text
>> +VALID=$D/valid-addrs
>> +ALL=$D/all-addrs
>> +BAD=$D/bad-addrs
>> +
>> +# Check that grep can do binary searches. Some, e.g. busybox, can't.
>> Leave a
>> +# warning but don't fail the build.
>> +echo "X" | grep -aob "X" -q 2>/dev/null ||
>> + { echo "$MSG_PFX Warning: grep can't do binary searches" >&2; exit 0; }
>> +
>> +#
>> +# First, look for all the valid endbr64 instructions.
>> +# A worst-case disassembly, viewed through cat -A, may look like:
>> +#
>> +# ffff82d040337bd4 <endbr64>:$
>> +# ffff82d040337bd4:^If3 0f 1e fa ^Iendbr64 $
>> +# ffff82d040337bd8:^Ieb fe ^Ijmp ffff82d040337bd8
>> <endbr64+0x4>$
>> +# ffff82d040337bda:^Ib8 f3 0f 1e fa ^Imov $0xfa1e0ff3,%eax$
>> +#
>> +# Want to grab the address of endbr64 instructions only, ignoring function
>> +# names/jump labels/etc, so look for 'endbr64' preceeded by a tab and with
>> any
>> +# number of trailing spaces before the end of the line.
>> +#
>> +${OBJDUMP} -d -w | grep ' endbr64 *$' | cut -f 1 -d ':' > $VALID &
>> +
>> +#
>> +# Second, look for any endbr64 byte sequence
>> +# 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.
>> +#
>> +# 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
>> +# the lower bits, rounding integers to the nearest 4k.
>> +#
>> +# Instead, use the fact that Xen's .text is within a 1G aligned region,
>> and
>> +# split the VMA in half so AWK's numeric addition is only working on 32
>> bit
>> +# numbers, which don't lose precision.
>> +#
>> +eval $(${OBJDUMP} -h | awk '$2 == ".text" {printf "vma_hi=%s\nvma_lo=%s\n",
>> substr($4, 1, 8), substr($4, 9, 16)}')
>> +
>> +${OBJCOPY} -O binary $TEXT_BIN
> ..., like here, is then somewhat misleading considering that the tool
> can take one or two filenames as arguments.
I can re-expand them if you'd prefer. This would be the delta:
diff --git a/xen/tools/check-endbr.sh b/xen/tools/check-endbr.sh
index 85878353112a..3019ca1c7db0 100755
--- a/xen/tools/check-endbr.sh
+++ b/xen/tools/check-endbr.sh
@@ -7,8 +7,8 @@ set -e
# Prettyprint parameters a little for message
MSG_PFX="${0##*/} ${1##*/}"
-OBJCOPY="${OBJCOPY:-objcopy} -j .text $1"
-OBJDUMP="${OBJDUMP:-objdump} -j .text $1"
+OBJCOPY="${OBJCOPY:-objcopy}"
+OBJDUMP="${OBJDUMP:-objdump}"
ADDR2LINE="${ADDR2LINE:-addr2line}"
D=$(mktemp -d)
@@ -37,7 +37,7 @@ echo "X" | grep -aob "X" -q 2>/dev/null ||
# names/jump labels/etc, so look for 'endbr64' preceeded by a tab and
with any
# number of trailing spaces before the end of the line.
#
-${OBJDUMP} -d -w | grep ' endbr64 *$' | cut -f 1 -d ':' > $VALID &
+${OBJDUMP} -j .text $1 -d -w | grep ' endbr64 *$' | cut -f 1 -d ':' >
$VALID &
#
# Second, look for any endbr64 byte sequence
@@ -56,9 +56,10 @@ ${OBJDUMP} -d -w | grep ' endbr64 *$' | cut -f 1
-d ':' > $VALID &
# split the VMA in half so AWK's numeric addition is only working on
32 bit
# numbers, which don't lose precision.
#
-eval $(${OBJDUMP} -h | awk '$2 == ".text" {printf
"vma_hi=%s\nvma_lo=%s\n", substr($4, 1, 8), substr($4, 9, 16)}')
+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} -O binary $TEXT_BIN
+${OBJCOPY} -j .text $1 -O binary $TEXT_BIN
grep -aob "$(printf '\363\17\36\372')" $TEXT_BIN |
awk -F':' '{printf "%s%x\n", "'$vma_hi'", int(0x'$vma_lo') + $1}' >
$ALL
~Andrew
