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

[Jan Beulich <jbeulich@xxxxxxxx>]

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.

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

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

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

Jan