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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 22.02.2022 16:26, Andrew Cooper wrote:
> From: Marek Marczykowski-Górecki <marmarek@xxxxxxxxxxxxxxxxxxxxxx>
> 
> An interesting corner case occurs when the byte sequence making up endb64 ends

Nit: For grep-ability it would be nice to spell this "endbr64".

> 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?

> 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@xxxxxxxxxxxxxxxxxxxxxx>
> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>

Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>

> --- 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?

> --- /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.

Jan