Re: [PATCH 59.5/65] x86: Introduce helpers/checks for endbr64 instructions

From: Andrew Cooper <amc96@srcf.net>
To: Jan Beulich <jbeulich@suse.com>,
	Andrew Cooper <andrew.cooper3@citrix.com>
Cc: "Roger Pau Monné" <roger.pau@citrix.com>, "Wei Liu" <wl@xen.org>,
	Xen-devel <xen-devel@lists.xenproject.org>
Subject: Re: [PATCH 59.5/65] x86: Introduce helpers/checks for endbr64 instructions
Date: Fri, 3 Dec 2021 14:10:56 +0000	[thread overview]
Message-ID: <ef297668-96ba-851d-2990-179f6dcb9f37@srcf.net> (raw)
In-Reply-To: <e5da1903-20bc-4523-9d0a-6071f117c3ce@suse.com>

On 03/12/2021 13:59, Jan Beulich wrote:
> On 26.11.2021 17:33, Andrew Cooper wrote:
>> ... to prevent the optimiser creating unsafe code.  See the code comment for
>> full details.
>>
>> Also add a build time check for endbr64 embedded in imm32 operands, which
>> catches the obvious cases where the optimiser has done an unsafe thing.
> But this is hardly enough to be safe. I'd even go as far as saying we can
> do without it if we don't check more thoroughly.

I will do the full check in v2.  Marek wrote the full check in response
to a discussion about this patch.

>
>> --- a/xen/arch/x86/Makefile
>> +++ b/xen/arch/x86/Makefile
>> @@ -190,6 +190,10 @@ $(TARGET)-syms: prelink.o xen.lds
>>  	$(MAKE) -f $(BASEDIR)/Rules.mk efi-y= $(@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)
>> +	$(OBJDUMP) -d $@ | grep 0xfa1e0ff3 >/dev/null && \
>> +		{ echo "Found embedded endbr64 instructions" >&2; false; } || :
> I guess I'm confused: The "false;" suggests to me you want to make the
> build fail in such a case. The "|| :" otoh suggests you want to silence
> errors (and not just the one from grep when not finding the pattern
> aiui).

The exit code of grep needs inverting for the build to proceed
correctly.  Without || :, all builds fail when they've not got the pattern.

> Also isn't passing -q to grep standard enough (and shorter) to use in
> place of redirecting its output to /dev/null?

That caused problems on the BSDs.  c/s e632d56f0f5 went through several
iterations before settling on this pattern.

>
>> --- /dev/null
>> +++ b/xen/include/asm-x86/endbr.h
>> @@ -0,0 +1,55 @@
>> +/******************************************************************************
>> + * include/asm-x86/endbr.h
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License as published by
>> + * the Free Software Foundation; either version 2 of the License, or
>> + * (at your option) any later version.
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> + * GNU General Public License for more details.
>> + *
>> + * You should have received a copy of the GNU General Public License
>> + * along with this program; If not, see <http://www.gnu.org/licenses/>.
>> + *
>> + * Copyright (c) 2021 Citrix Systems Ltd.
>> + */
>> +#ifndef XEN_ASM_ENDBR_H
>> +#define XEN_ASM_ENDBR_H
>> +
>> +#include <xen/compiler.h>
>> +
>> +/*
>> + * In some cases we need to inspect/insert endbr64 instructions.
>> + *
>> + * The naive way, mem{cmp,cpy}(ptr, "\xf3\x0f\x1e\xfa", 4), optimises unsafely
>> + * by placing 0xfa1e0ff3 in an imm32 operand, which marks a legal indirect
>> + * branch target as far as the CPU is concerned.
>> + *
>> + * gen_endbr64() is written deliberately to avoid the problematic operand, and
>> + * marked __const__ as it is safe for the optimiser to hoist/merge/etc.
>> + */
>> +static inline uint32_t __attribute_const__ gen_endbr64(void)
>> +{
>> +    uint32_t res;
>> +
>> +    asm ( "mov $~0xfa1e0ff3, %[res]\n\t"
>> +          "not %[res]\n\t"
>> +          : [res] "=r" (res) );
> Strictly speaking "=&r".

Ok.

~Andrew