x86: Introduce helpers/checks for endbr64 instructions

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

Posted by Andrew Cooper 3 years ago

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

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Wei Liu <wl@xen.org>
---
 xen/arch/x86/Makefile       |  4 ++++
 xen/include/asm-x86/endbr.h | 55 +++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 59 insertions(+)
 create mode 100644 xen/include/asm-x86/endbr.h

diff --git a/xen/arch/x86/Makefile b/xen/arch/x86/Makefile
index 69b6cfaded25..64a5c0d20018 100644
--- 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; } || :
+endif
 	$(NM) -pa --format=sysv $(@D)/$(@F) \
 		| $(BASEDIR)/tools/symbols --all-symbols --xensyms --sysv --sort \
 		>$(@D)/$(@F).map
diff --git a/xen/include/asm-x86/endbr.h b/xen/include/asm-x86/endbr.h
new file mode 100644
index 000000000000..47f766024c12
--- /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) );
+
+    return res;
+}
+
+static inline bool is_endbr64(const void *ptr)
+{
+    return *(const uint32_t *)ptr == gen_endbr64();
+}
+
+static inline void place_endbr64(void *ptr)
+{
+    *(uint32_t *)ptr = gen_endbr64();
+}
+
+#endif /* XEN_ASM_ENDBR_H */
-- 
2.11.0

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

Posted by Marek Marczykowski-Górecki 3 years ago

On Fri, Nov 26, 2021 at 04:33:40PM +0000, 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.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> ---
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Roger Pau Monné <roger.pau@citrix.com>
> CC: Wei Liu <wl@xen.org>
> ---
>  xen/arch/x86/Makefile       |  4 ++++
>  xen/include/asm-x86/endbr.h | 55 +++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 59 insertions(+)
>  create mode 100644 xen/include/asm-x86/endbr.h
> 
> diff --git a/xen/arch/x86/Makefile b/xen/arch/x86/Makefile
> index 69b6cfaded25..64a5c0d20018 100644
> --- 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; } || :
> +endif

Some more robust check can be done this way (warning, PoC quality bash):

    objcopy -j .text xen-syms xen-syms.text
    offset=$(objdump -h xen-syms -j .text | tail -2|head -1|awk '{printf "%x\n", (strtonum("0x" $4) - strtonum("0x" $6))}')
    objdump --adjust-vma=-0x$offset -d xen-syms.text|grep endbr | cut -f 1 -d ':' | tr -d ' ' > valid-addrs
    grep -aob $'\xf3\x0f\x1e\xfa' xen-syms.text|cut -f 1 -d :|xargs printf '%x\n' > all-addrs
    join -v 2 <(sort valid-addrs) <(sort all-addrs) | awk '{ printf "%x\n", 0x'$offset' + strtonum("0x" $1)}' | addr2line -e xen-syms

Currently it finds just one match:
xen/arch/x86/alternative.c:145

-- 
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab

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

Posted by Andrew Cooper 3 years ago

On 26/11/2021 18:26, Marek Marczykowski-Górecki wrote:
> On Fri, Nov 26, 2021 at 04:33:40PM +0000, 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.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>> ---
>> CC: Jan Beulich <JBeulich@suse.com>
>> CC: Roger Pau Monné <roger.pau@citrix.com>
>> CC: Wei Liu <wl@xen.org>
>> ---
>>  xen/arch/x86/Makefile       |  4 ++++
>>  xen/include/asm-x86/endbr.h | 55 +++++++++++++++++++++++++++++++++++++++++++++
>>  2 files changed, 59 insertions(+)
>>  create mode 100644 xen/include/asm-x86/endbr.h
>>
>> diff --git a/xen/arch/x86/Makefile b/xen/arch/x86/Makefile
>> index 69b6cfaded25..64a5c0d20018 100644
>> --- 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; } || :
>> +endif
> Some more robust check can be done this way (warning, PoC quality bash):
>
>     objcopy -j .text xen-syms xen-syms.text
>     offset=$(objdump -h xen-syms -j .text | tail -2|head -1|awk '{printf "%x\n", (strtonum("0x" $4) - strtonum("0x" $6))}')
>     objdump --adjust-vma=-0x$offset -d xen-syms.text|grep endbr | cut -f 1 -d ':' | tr -d ' ' > valid-addrs
>     grep -aob $'\xf3\x0f\x1e\xfa' xen-syms.text|cut -f 1 -d :|xargs printf '%x\n' > all-addrs
>     join -v 2 <(sort valid-addrs) <(sort all-addrs) | awk '{ printf "%x\n", 0x'$offset' + strtonum("0x" $1)}' | addr2line -e xen-syms
>
> Currently it finds just one match:
> xen/arch/x86/alternative.c:145

To be clear, this one match is on the xen-cet-ibt v1.1 branch, which
also includes the next task (runtime clobbering of unused ENDBR
instructions) which I'm currently cleaning up to post.

~Andrew

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

Posted by Jan Beulich 2 years, 12 months ago

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

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

Posted by Andrew Cooper 2 years, 12 months ago

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

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

Posted by Jan Beulich 2 years, 11 months ago

On 03.12.2021 15:10, Andrew Cooper wrote:
> On 03/12/2021 13:59, Jan Beulich wrote:
>> On 26.11.2021 17:33, Andrew Cooper wrote:
>>> --- 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.

But doesn't this invert not only failure of grep, but also the
unconditional "failure" of "false"? IOW doesn't this step therefore
always succeed, making the message merely a warning-like one? Or if
that's the intended behavior, what's the "false" good for?

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

Odd. The commit message doesn't mention any of this. v2 of that patch
already didn't use -q, and also doesn't identify a respective change
from v1. I wasn't able to locate v1.

Jan