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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CAKwvOdkyi1c9TvP_Bzn3+71kKsJzAbQZmgDzfDM8c7sce-V0Dw@mail.gmail.com>
Date:   Thu, 10 Mar 2022 12:49:29 -0800
From:   Nick Desaulniers <ndesaulniers@...gle.com>
To:     Peter Zijlstra <peterz@...radead.org>
Cc:     David Laight <David.Laight@...lab.com>,
        Alexei Starovoitov <alexei.starovoitov@...il.com>,
        "x86@...nel.org" <x86@...nel.org>,
        "joao@...rdrivepizza.com" <joao@...rdrivepizza.com>,
        "hjl.tools@...il.com" <hjl.tools@...il.com>,
        "jpoimboe@...hat.com" <jpoimboe@...hat.com>,
        "andrew.cooper3@...rix.com" <andrew.cooper3@...rix.com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "keescook@...omium.org" <keescook@...omium.org>,
        "samitolvanen@...gle.com" <samitolvanen@...gle.com>,
        "mark.rutland@....com" <mark.rutland@....com>,
        "alyssa.milburn@...el.com" <alyssa.milburn@...el.com>,
        "mbenes@...e.cz" <mbenes@...e.cz>,
        "rostedt@...dmis.org" <rostedt@...dmis.org>,
        "mhiramat@...nel.org" <mhiramat@...nel.org>,
        "daniel@...earbox.net" <daniel@...earbox.net>,
        "andrii@...nel.org" <andrii@...nel.org>,
        "bpf@...r.kernel.org" <bpf@...r.kernel.org>,
        "llvm@...ts.linux.dev" <llvm@...ts.linux.dev>
Subject: Re: [PATCH v4 00/45] x86: Kernel IBT

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

For the kernel IBT series @ v4 plus this diff:

Tested-by: Nick Desaulniers <ndesaulniers@...gle.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