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

[Nick Desaulniers]

On Thu, Mar 10, 2022 at 2:16 AM Peter Zijlstra <peterz@xxxxxxxxxxxxx> 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@xxxxxxxxxx>
Reviewed-by: Nick Desaulniers <ndesaulniers@xxxxxxxxxx>

For the kernel IBT series @ v4 plus this diff:

Tested-by: Nick Desaulniers <ndesaulniers@xxxxxxxxxx> # 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