Re: [PATCH v2 07/70] x86: Build check for embedded endbr64 instructions

[Jan Beulich <jbeulich@xxxxxxxx>]

On 14.02.2022 13:50, Andrew Cooper wrote:
> From: Marek Marczykowski-Górecki <marmarek@xxxxxxxxxxxxxxxxxxxxxx>
> 
> Embedded endbr64 instructions mark legal indirect branches as far as the CPU
> is concerned, which aren't legal as far as the logic is concerned.

I think it would help if it was clarified what "embedded" actually means
here.

> --- a/xen/arch/x86/Makefile
> +++ b/xen/arch/x86/Makefile
> @@ -155,6 +155,9 @@ $(TARGET)-syms: prelink.o xen.lds
>       $(MAKE) -f $(BASEDIR)/Rules.mk $(@D)/.$(@F).1.o
>       $(LD) $(XEN_LDFLAGS) -T xen.lds -N prelink.o $(build_id_linker) \
>           $(@D)/.$(@F).1.o -o $@
> +ifeq ($(CONFIG_XEN_IBT),y)
> +     $(SHELL) $(BASEDIR)/tools/check-endbr.sh $@
> +endif
>       $(NM) -pa --format=sysv $(@D)/$(@F) \
>               | $(BASEDIR)/tools/symbols --all-symbols --xensyms --sysv 
> --sort \
>               >$(@D)/$(@F).map

The same wants doing on xen.efi, I guess?

> --- /dev/null
> +++ b/xen/tools/check-endbr.sh
> @@ -0,0 +1,76 @@
> +#!/bin/sh
> +
> +#
> +# Usage ./$0 xen-syms
> +#
> +
> +set -e
> +
> +OBJCOPY="${OBJCOPY:-objcopy} -j .text $1"
> +OBJDUMP="${OBJDUMP:-objdump} -j .text $1"
> +
> +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
> +
> +#
> +# 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 | grep '       endbr64 *$' | cut -f 1 -d ':' > $VALID &

Since you look at only .text the risk of the disassembler coming
out of sync with the actual instruction stream is lower than when
32- and 16-bit code was also part of what is disassembled, but it's
not zero. Any zero-padding inserted anywhere by the linker can
result in an immediately following ENDBR to be missed (because
sequences of zeros resemble 2-byte insns). While this risk may be
acceptable, I think it wants mentioning at least in the description,
maybe even at the top of the script (where one would likely look
first after it spitting out an error).

Do you perhaps want to also pass -w to objdump, to eliminate the
risk of getting confused by split lines?

> +#
> +# 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) 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
> +grep -aob "$(printf '\363\17\36\372')" $TEXT_BIN |
> +    awk -F':' '{printf "%s%x\n", "'$vma_hi'", strtonum(0x'$vma_lo') + $1}' > 
> $ALL

None of the three options passed to grep look to be standardized.
Is this going to cause problems on non-Linux systems? Should this
checking perhaps be put behind a separate Kconfig option?

> +# Wait for $VALID to become complete
> +wait
> +
> +# Sanity check $VALID and $ALL, in case the string parsing bitrots
> +val_sz=$(stat -c '%s' $VALID)
> +all_sz=$(stat -c '%s' $ALL)
> +[ "$val_sz" -eq 0 ]         && { echo "Error: Empty valid-addrs" >&2; exit 
> 1; }
> +[ "$all_sz" -eq 0 ]         && { echo "Error: Empty all-addrs" >&2; exit 1; }
> +[ "$all_sz" -lt "$val_sz" ] && { echo "Error: More valid-addrs than 
> all-addrs" >&2; exit 1; }
> +
> +# $BAD = $ALL - $VALID
> +join -v 2 $VALID $ALL > $BAD
> +nr_bad=$(wc -l < $BAD)
> +
> +# Success
> +[ "$nr_bad" -eq 0 ] && exit 0
> +
> +# Failure
> +echo "Fail: Found ${nr_bad} embedded endbr64 instructions" >&2
> +addr2line -afip -e $1 < $BAD >&2

There probably also wants to be an ADDR2LINE variable then. If
one overrides objdump and objcopy, one would likely want/need to
override this one as well.

Jan