Re: [PATCH v3 60/70] x86: Build check for embedded endbr64 instructions

From: Andrew Cooper <Andrew.Cooper3@citrix.com>
To: Jan Beulich <jbeulich@suse.com>
Cc: "Marek Marczykowski-Górecki" <marmarek@invisiblethingslab.com>,
	"Roger Pau Monne" <roger.pau@citrix.com>, "Wei Liu" <wl@xen.org>,
	Xen-devel <xen-devel@lists.xenproject.org>
Subject: Re: [PATCH v3 60/70] x86: Build check for embedded endbr64 instructions
Date: Wed, 23 Feb 2022 12:05:09 +0000	[thread overview]
Message-ID: <505a8553-5930-762b-ed07-f8f49df20064@citrix.com> (raw)
In-Reply-To: <48241c60-294b-782f-7217-45a3dc929c38@suse.com>

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 <marmarek@invisiblethingslab.com>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Reviewed-by: Jan Beulich <jbeulich@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
