Re: [PATCH v4 00/45] x86: Kernel IBT

Re: [PATCH v4 00/45] x86: Kernel IBT

[Nick Desaulniers]

On Tue, Mar 8, 2022 at 2:32 PM Peter Zijlstra <peterz@infradead.org> wrote:
>
> On Tue, Mar 08, 2022 at 11:01:04PM +0100, Peter Zijlstra wrote:
> > On Tue, Mar 08, 2022 at 12:00:52PM -0800, Alexei Starovoitov wrote:
> > > On Tue, Mar 08, 2022 at 04:30:11PM +0100, Peter Zijlstra wrote:
> > > > Hopefully last posting...
> > > >
> > > > Since last time:
> > > >  - verified clang-14-rc2 works
> > > >   git://git.kernel.org/pub/scm/linux/kernel/git/peterz/queue.git x86/wip.ibt

I observed the following error when building with
CONFIG_LTO_CLANG_FULL=y enabled:

ld.lld: error: ld-temp.o <inline asm>:7:2: symbol 'ibt_selftest_ip' is
already defined
        ibt_selftest_ip:
        ^

Seems to come from
commit a802350ba65a ("x86/ibt: Add IBT feature, MSR and #CP handling")

Commenting out the label in the inline asm, I then observed:
vmlinux.o: warning: objtool: identify_cpu()+0x6d0: sibling call from
callable instruction with modified stack frame
vmlinux.o: warning: objtool: identify_cpu()+0x6e0: stack state
mismatch: cfa1=4+64 cfa2=4+8
These seemed to disappear when I kept CONFIG_LTO_CLANG_FULL=y but then
disabled CONFIG_X86_KERNEL_IBT. (perhaps due to the way I hacked out
the ibt_selftest_ip label).

Otherwise defconfig and CONFIG_LTO_CLANG_THIN=y both built and booted
in a vm WITHOUT IBT support.

Any idea what's the status of IBT emulation in QEMU, and if it exists,
what's the necessary `-cpu` flag to enable it?
-- 
Thanks,
~Nick Desaulniers

Re: [PATCH v4 00/45] x86: Kernel IBT

[Peter Zijlstra]

On Wed, Mar 09, 2022 at 04:30:28PM -0800, Nick Desaulniers wrote:

> I observed the following error when building with
> CONFIG_LTO_CLANG_FULL=y enabled:
> 
> ld.lld: error: ld-temp.o <inline asm>:7:2: symbol 'ibt_selftest_ip' is
> already defined
>         ibt_selftest_ip:
>         ^
> 
> Seems to come from
> commit a802350ba65a ("x86/ibt: Add IBT feature, MSR and #CP handling")
> 
> Commenting out the label in the inline asm, I then observed:
> vmlinux.o: warning: objtool: identify_cpu()+0x6d0: sibling call from
> callable instruction with modified stack frame
> vmlinux.o: warning: objtool: identify_cpu()+0x6e0: stack state
> mismatch: cfa1=4+64 cfa2=4+8
> These seemed to disappear when I kept CONFIG_LTO_CLANG_FULL=y but then
> disabled CONFIG_X86_KERNEL_IBT. (perhaps due to the way I hacked out
> the ibt_selftest_ip label).

Urgh.. I'm thikning this is a clang bug :/

The code in question is:


void ibt_selftest_ip(void); /* code label defined in asm below */

DEFINE_IDTENTRY_ERRORCODE(exc_control_protection)
{
	/* ... */

	if (unlikely(regs->ip == (unsigned long)ibt_selftest_ip)) {
		regs->ax = 0;
		return;
	}

	/* ... */
}

bool ibt_selftest(void)
{
	unsigned long ret;

	asm ("	lea ibt_selftest_ip(%%rip), %%rax\n\t"
	     ANNOTATE_RETPOLINE_SAFE
	     "	jmp *%%rax\n\t"
	     "ibt_selftest_ip:\n\t"
	     UNWIND_HINT_FUNC
	     ANNOTATE_NOENDBR
	     "	nop\n\t"

	     : "=a" (ret) : : "memory");

	return !ret;
}

There is only a single definition of that symbol, the one in the asm.
The other is a declaration, which is used in the exception handler to
compare against regs->ip.

So what this code does is trigger an explicit #CP and special case that
in the handler. For that the handler needs to know the special IP that
will trigger the failure, this is cummunicated with that symbol.

> Otherwise defconfig and CONFIG_LTO_CLANG_THIN=y both built and booted
> in a vm WITHOUT IBT support.
> 
> Any idea what's the status of IBT emulation in QEMU, and if it exists,
> what's the necessary `-cpu` flag to enable it?

I have a very ugly kvm patch that goes with a very ugly qemu patch to
make it work. I would very much not recommend those getting merged.

Someone with some actual kvm/qemu foo should do one. The complicating
factor is that IA32_S_CET also contains SHSTK enable bits, so a straight
passthrough like I use relies on the guest never setting those bits or
keeping the pieces. It either needs to filter the MSR or implement the
full CET mess.

RE: [PATCH v4 00/45] x86: Kernel IBT

[David Laight]

From: Peter Zijlstra
> Sent: 10 March 2022 09:05
> 
> On Wed, Mar 09, 2022 at 04:30:28PM -0800, Nick Desaulniers wrote:
> 
> > I observed the following error when building with
> > CONFIG_LTO_CLANG_FULL=y enabled:
> >
> > ld.lld: error: ld-temp.o <inline asm>:7:2: symbol 'ibt_selftest_ip' is
> > already defined
> >         ibt_selftest_ip:
> >         ^
> >
> > Seems to come from
> > commit a802350ba65a ("x86/ibt: Add IBT feature, MSR and #CP handling")
> >
> > Commenting out the label in the inline asm, I then observed:
> > vmlinux.o: warning: objtool: identify_cpu()+0x6d0: sibling call from
> > callable instruction with modified stack frame
> > vmlinux.o: warning: objtool: identify_cpu()+0x6e0: stack state
> > mismatch: cfa1=4+64 cfa2=4+8
> > These seemed to disappear when I kept CONFIG_LTO_CLANG_FULL=y but then
> > disabled CONFIG_X86_KERNEL_IBT. (perhaps due to the way I hacked out
> > the ibt_selftest_ip label).
> 
> Urgh.. I'm thikning this is a clang bug :/
> 
> The code in question is:
> 
> 
> void ibt_selftest_ip(void); /* code label defined in asm below */
> 
> DEFINE_IDTENTRY_ERRORCODE(exc_control_protection)
> {
> 	/* ... */
> 
> 	if (unlikely(regs->ip == (unsigned long)ibt_selftest_ip)) {
> 		regs->ax = 0;
> 		return;
> 	}
> 
> 	/* ... */
> }
> 
> bool ibt_selftest(void)
> {
> 	unsigned long ret;
> 
> 	asm ("	lea ibt_selftest_ip(%%rip), %%rax\n\t"
> 	     ANNOTATE_RETPOLINE_SAFE
> 	     "	jmp *%%rax\n\t"
> 	     "ibt_selftest_ip:\n\t"
> 	     UNWIND_HINT_FUNC
> 	     ANNOTATE_NOENDBR
> 	     "	nop\n\t"
> 
> 	     : "=a" (ret) : : "memory");
> 
> 	return !ret;
> }
> 
> There is only a single definition of that symbol, the one in the asm.
> The other is a declaration, which is used in the exception handler to
> compare against regs->ip.

LTO has probably inlined it twice.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

Re: [PATCH v4 00/45] x86: Kernel IBT

[Peter Zijlstra]

On Thu, Mar 10, 2022 at 09:22:59AM +0000, David Laight wrote:
> From: Peter Zijlstra
> > Sent: 10 March 2022 09:05
> > 
> > On Wed, Mar 09, 2022 at 04:30:28PM -0800, Nick Desaulniers wrote:
> > 
> > > I observed the following error when building with
> > > CONFIG_LTO_CLANG_FULL=y enabled:
> > >
> > > ld.lld: error: ld-temp.o <inline asm>:7:2: symbol 'ibt_selftest_ip' is
> > > already defined
> > >         ibt_selftest_ip:
> > >         ^
> > >
> > > Seems to come from
> > > commit a802350ba65a ("x86/ibt: Add IBT feature, MSR and #CP handling")
> > >
> > > Commenting out the label in the inline asm, I then observed:
> > > vmlinux.o: warning: objtool: identify_cpu()+0x6d0: sibling call from
> > > callable instruction with modified stack frame
> > > vmlinux.o: warning: objtool: identify_cpu()+0x6e0: stack state
> > > mismatch: cfa1=4+64 cfa2=4+8
> > > These seemed to disappear when I kept CONFIG_LTO_CLANG_FULL=y but then
> > > disabled CONFIG_X86_KERNEL_IBT. (perhaps due to the way I hacked out
> > > the ibt_selftest_ip label).
> > 
> > Urgh.. I'm thikning this is a clang bug :/
> > 
> > The code in question is:
> > 
> > 
> > void ibt_selftest_ip(void); /* code label defined in asm below */
> > 
> > DEFINE_IDTENTRY_ERRORCODE(exc_control_protection)
> > {
> > 	/* ... */
> > 
> > 	if (unlikely(regs->ip == (unsigned long)ibt_selftest_ip)) {
> > 		regs->ax = 0;
> > 		return;
> > 	}
> > 
> > 	/* ... */
> > }
> > 
> > bool ibt_selftest(void)
> > {
> > 	unsigned long ret;
> > 
> > 	asm ("	lea ibt_selftest_ip(%%rip), %%rax\n\t"
> > 	     ANNOTATE_RETPOLINE_SAFE
> > 	     "	jmp *%%rax\n\t"
> > 	     "ibt_selftest_ip:\n\t"
> > 	     UNWIND_HINT_FUNC
> > 	     ANNOTATE_NOENDBR
> > 	     "	nop\n\t"
> > 
> > 	     : "=a" (ret) : : "memory");
> > 
> > 	return !ret;
> > }
> > 
> > There is only a single definition of that symbol, the one in the asm.
> > The other is a declaration, which is used in the exception handler to
> > compare against regs->ip.
> 
> LTO has probably inlined it twice.

Indeed, adding noinline to ibt_selftest() makes it work.


---
diff --git a/arch/x86/kernel/alternative.c b/arch/x86/kernel/alternative.c
index d8bbc705efe5..0c737cc31ee5 100644
--- a/arch/x86/kernel/alternative.c
+++ b/arch/x86/kernel/alternative.c
@@ -781,7 +781,8 @@ int3_exception_notify(struct notifier_block *self, unsigned long val, void *data
 	return NOTIFY_STOP;
 }
 
-static void __init int3_selftest(void)
+/* Must be noinline to ensure uniqueness of int3_selftest_ip. */
+static noinline void __init int3_selftest(void)
 {
 	static __initdata struct notifier_block int3_exception_nb = {
 		.notifier_call	= int3_exception_notify,
@@ -794,9 +795,8 @@ static void __init int3_selftest(void)
 	/*
 	 * Basically: int3_magic(&val); but really complicated :-)
 	 *
-	 * Stick the address of the INT3 instruction into int3_selftest_ip,
-	 * then trigger the INT3, padded with NOPs to match a CALL instruction
-	 * length.
+	 * INT3 padded with NOP to CALL_INSN_SIZE. The int3_exception_nb
+	 * notifier above will emulate CALL for us.
 	 */
 	asm volatile ("int3_selftest_ip:\n\t"
 		      ANNOTATE_NOENDBR
diff --git a/arch/x86/kernel/traps.c b/arch/x86/kernel/traps.c
index 837cc3c7d4f4..fb89a2f1011f 100644
--- a/arch/x86/kernel/traps.c
+++ b/arch/x86/kernel/traps.c
@@ -214,7 +214,7 @@ DEFINE_IDTENTRY(exc_overflow)
 
 static __ro_after_init bool ibt_fatal = true;
 
-void ibt_selftest_ip(void); /* code label defined in asm below */
+extern void ibt_selftest_ip(void); /* code label defined in asm below */
 
 enum cp_error_code {
 	CP_EC        = (1 << 15) - 1,
@@ -238,7 +238,7 @@ DEFINE_IDTENTRY_ERRORCODE(exc_control_protection)
 	if (WARN_ON_ONCE(user_mode(regs) || (error_code & CP_EC) != CP_ENDBR))
 		return;
 
-	if (unlikely(regs->ip == (unsigned long)ibt_selftest_ip)) {
+	if (unlikely(regs->ip == (unsigned long)&ibt_selftest_ip)) {
 		regs->ax = 0;
 		return;
 	}
@@ -252,7 +252,8 @@ DEFINE_IDTENTRY_ERRORCODE(exc_control_protection)
 	BUG();
 }
 
-bool ibt_selftest(void)
+/* Must be noinline to ensure uniqueness of ibt_selftest_ip. */
+noinline bool ibt_selftest(void)
 {
 	unsigned long ret;
 

Re: [PATCH v4 00/45] x86: Kernel IBT

[Nick Desaulniers]

On Thu, Mar 10, 2022 at 2:16 AM Peter Zijlstra <peterz@infradead.org> wrote:
>
> On Thu, Mar 10, 2022 at 09:22:59AM +0000, David Laight wrote:
> > From: Peter Zijlstra
> > > Sent: 10 March 2022 09:05
> > >
> > > On Wed, Mar 09, 2022 at 04:30:28PM -0800, Nick Desaulniers wrote:
> > >
> > > > I observed the following error when building with
> > > > CONFIG_LTO_CLANG_FULL=y enabled:
> > > >
> > > > ld.lld: error: ld-temp.o <inline asm>:7:2: symbol 'ibt_selftest_ip' is
> > > > already defined
> > > >         ibt_selftest_ip:
> > > >         ^
> > > >
> > > > Seems to come from
> > > > commit a802350ba65a ("x86/ibt: Add IBT feature, MSR and #CP handling")
> > > >
> > > > Commenting out the label in the inline asm, I then observed:
> > > > vmlinux.o: warning: objtool: identify_cpu()+0x6d0: sibling call from
> > > > callable instruction with modified stack frame
> > > > vmlinux.o: warning: objtool: identify_cpu()+0x6e0: stack state
> > > > mismatch: cfa1=4+64 cfa2=4+8
> > > > These seemed to disappear when I kept CONFIG_LTO_CLANG_FULL=y but then
> > > > disabled CONFIG_X86_KERNEL_IBT. (perhaps due to the way I hacked out
> > > > the ibt_selftest_ip label).
> > >
> > LTO has probably inlined it twice.
>
> Indeed, adding noinline to ibt_selftest() makes it work.

Yep, that LGTM. If you end up sticking that as a patch on top:

Reported-by: Nick Desaulniers <ndesaulniers@google.com>
Reviewed-by: Nick Desaulniers <ndesaulniers@google.com>

For the kernel IBT series @ v4 plus this diff:

Tested-by: Nick Desaulniers <ndesaulniers@google.com> # llvm build, non-IBT boot

>
>
> ---
> diff --git a/arch/x86/kernel/alternative.c b/arch/x86/kernel/alternative.c
> index d8bbc705efe5..0c737cc31ee5 100644
> --- a/arch/x86/kernel/alternative.c
> +++ b/arch/x86/kernel/alternative.c
> @@ -781,7 +781,8 @@ int3_exception_notify(struct notifier_block *self, unsigned long val, void *data
>         return NOTIFY_STOP;
>  }
>
> -static void __init int3_selftest(void)
> +/* Must be noinline to ensure uniqueness of int3_selftest_ip. */
> +static noinline void __init int3_selftest(void)
>  {
>         static __initdata struct notifier_block int3_exception_nb = {
>                 .notifier_call  = int3_exception_notify,
> @@ -794,9 +795,8 @@ static void __init int3_selftest(void)
>         /*
>          * Basically: int3_magic(&val); but really complicated :-)
>          *
> -        * Stick the address of the INT3 instruction into int3_selftest_ip,
> -        * then trigger the INT3, padded with NOPs to match a CALL instruction
> -        * length.
> +        * INT3 padded with NOP to CALL_INSN_SIZE. The int3_exception_nb
> +        * notifier above will emulate CALL for us.
>          */
>         asm volatile ("int3_selftest_ip:\n\t"
>                       ANNOTATE_NOENDBR
> diff --git a/arch/x86/kernel/traps.c b/arch/x86/kernel/traps.c
> index 837cc3c7d4f4..fb89a2f1011f 100644
> --- a/arch/x86/kernel/traps.c
> +++ b/arch/x86/kernel/traps.c
> @@ -214,7 +214,7 @@ DEFINE_IDTENTRY(exc_overflow)
>
>  static __ro_after_init bool ibt_fatal = true;
>
> -void ibt_selftest_ip(void); /* code label defined in asm below */
> +extern void ibt_selftest_ip(void); /* code label defined in asm below */
>
>  enum cp_error_code {
>         CP_EC        = (1 << 15) - 1,
> @@ -238,7 +238,7 @@ DEFINE_IDTENTRY_ERRORCODE(exc_control_protection)
>         if (WARN_ON_ONCE(user_mode(regs) || (error_code & CP_EC) != CP_ENDBR))
>                 return;
>
> -       if (unlikely(regs->ip == (unsigned long)ibt_selftest_ip)) {
> +       if (unlikely(regs->ip == (unsigned long)&ibt_selftest_ip)) {

(Though adding the address of operator & to the function name in the
comparisons isn't strictly necessary; functions used in expressions
"decay" into function pointers; I guess the standard calls these
"function designators." I see that's been added to be consistent
between the two...See 6.3.2.1.4 of
http://open-std.org/jtc1/sc22/wg14/www/docs/n2731.pdf pdf page
62/printed page 46.)

>                 regs->ax = 0;
>                 return;
>         }
> @@ -252,7 +252,8 @@ DEFINE_IDTENTRY_ERRORCODE(exc_control_protection)
>         BUG();
>  }
>
> -bool ibt_selftest(void)
> +/* Must be noinline to ensure uniqueness of ibt_selftest_ip. */
> +noinline bool ibt_selftest(void)
>  {
>         unsigned long ret;
>


-- 
Thanks,
~Nick Desaulniers