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

From: Jan Beulich <jbeulich@suse.com>
To: Andrew Cooper <amc96@srcf.net>,
	Andrew Cooper <andrew.cooper3@citrix.com>
Cc: Xen-devel <xen-devel@lists.xenproject.org>,
	"Marek Marczykowski-Górecki" <marmarek@invisiblethingslab.com>
Subject: Re: [PATCH v2 07/70] x86: Build check for embedded endbr64 instructions
Date: Wed, 16 Feb 2022 09:41:44 +0100	[thread overview]
Message-ID: <7c5ff089-780b-9965-4765-be3ef210a913@suse.com> (raw)
In-Reply-To: <8c6d9714-7e45-1603-a5ad-d49fc060304e@srcf.net>

On 15.02.2022 18:52, Andrew Cooper wrote:
> On 15/02/2022 15:12, Jan Beulich wrote:
>> On 14.02.2022 13:50, Andrew Cooper wrote:
>>> --- /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.
> 
> I'm not sure that we have any interesting non-64bit code at all in .text.
> 
> _start is technically 32bit but is mode-invariant as far as decoding goes.
> 
> The kexec trampoline is here too, but when I dust off my cleanup patch,
> there will no longer be data or mode-dependent things to disassemble.
> 
> Everything else I can think of is in .init.text.
> 
>> 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).
> 
> I'm not sure this is a problem.  This pass is looking for everything
> that objdump thinks is a legal endbr64 instruction, and it splits at labels.

Oh, right - I did miss the splitting at labels aspect. Hopefully
objdump is really consistent with this.

> Only the hand-written stubs can legitimately have an endbr64 without a
> symbol pointing at it.
> 
> We also don't have any 0 padding.  It's specified as 0x90 in the linker
> file, although I've been debating switching this to 0xcc for a while now
> already.

The linker script comes into play only in the final linking step.
Prior "ld -r" could easily have inserted other padding.

>>> +#
>>> +# 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?
> 
> CI says that FreeBSD is entirely happy, while Alpine Linux isn't.  This
> is because Alpine has busybox's grep unless you install the GNU grep
> package, and I'm doing a fix to our container.
> 
> My plan to fix this is to just declare a "grep capable of binary
> searching" a conditional build requirement for Xen.  I don't think this
> is onerous, and there no other plausible alternatives here.
> 
> The other option is to detect the absence of support an skip the check. 
> It is after all a defence in depth scheme, and anything liable to cause
> a problem would be caught in CI anyway.

I'd favor the latter approach (but I wouldn't mind the conditional build
requirement, if you and others deem that better), with a warning issued
when the check can't be performed. I have to admit that I didn't expect
there would be no simple and standardized binary search tool on Unix-es.

Jan