Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20211013181658.1020262-1-samitolvanen@google.com>
 <20211026201622.GG174703@worktop.programming.kicks-ass.net>
 <20211027120515.GC54628@C02TD0UTHF1T.local> <CAMj1kXEx10gC8eH7rV-GbZZj2M3uDue6HYsKb+A5J01zOxm_FA@mail.gmail.com>
 <20211027124852.GK174703@worktop.programming.kicks-ass.net>
 <YXlOd1lyKZKAcJfA@hirez.programming.kicks-ass.net> <CAMj1kXHKh7wv6JqusVnoiQDMm7ApFq2ujzbfWmM9AzLKFehhAA@mail.gmail.com>
 <YXlcMluaysPBF92J@hirez.programming.kicks-ass.net> <CAMj1kXECTdDLVMk2JduU5mV2TR0Cv=hZ9QOpYRsRM1jfvvNikw@mail.gmail.com>
 <CABCJKufpS4jJxHqk8=bd1JCNbKfmLDKBbjbhjrar2+YQJFiprg@mail.gmail.com> <20211029200324.GR174703@worktop.programming.kicks-ass.net>
In-Reply-To: <20211029200324.GR174703@worktop.programming.kicks-ass.net>
From:   Sami Tolvanen <samitolvanen@google.com>
Date:   Sat, 30 Oct 2021 12:07:56 -0700
Message-ID: <CABCJKudsK9cLx90yAUVE07fTSqt605fmmK=tOU2zPgggz-M-QA@mail.gmail.com>
Subject: Re: [PATCH v5 00/15] x86: Add support for Clang CFI
To:     Peter Zijlstra <peterz@infradead.org>
Cc:     Ard Biesheuvel <ardb@kernel.org>,
        Mark Rutland <mark.rutland@arm.com>, X86 ML <x86@kernel.org>,
        Kees Cook <keescook@chromium.org>,
        Josh Poimboeuf <jpoimboe@redhat.com>,
        Nathan Chancellor <nathan@kernel.org>,
        Nick Desaulniers <ndesaulniers@google.com>,
        Sedat Dilek <sedat.dilek@gmail.com>,
        Steven Rostedt <rostedt@goodmis.org>,
        linux-hardening@vger.kernel.org,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        llvm@lists.linux.dev
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Fri, Oct 29, 2021 at 1:04 PM Peter Zijlstra <peterz@infradead.org> wrote:
>
> On Wed, Oct 27, 2021 at 08:50:17AM -0700, Sami Tolvanen wrote:
> > On Wed, Oct 27, 2021 at 7:18 AM Ard Biesheuvel <ardb@kernel.org> wrote:
>
> > > > /*
> > > >  * Turns a Clang CFI jump-table entry into an actual function pointer.
> > > >  * These jump-table entries are simply jmp.d32 instruction with their
> > > >  * relative offset pointing to the actual function, therefore decode the
> > > >  * instruction to find the real function.
> > > >  */
> > > > static __always_inline void *nocfi_ptr(void *func)
> > > > {
> > > >         union text_poke_insn insn = *(union text_poke_insn *)func;
> > > >
> > > >         return func + sizeof(insn) + insn.disp;
> > > > }
> > > >
> > > > But really, that wants to be a compiler intrinsic.
> > >
> > > Agreed. We could easily do something similar on arm64, but I'd prefer
> > > to avoid that too.
> >
> > I'll see what we can do. Note that the compiler built-in we previously
> > discussed would have semantics similar to function_nocfi(). It would
> > return the raw function address from a symbol name, but it wouldn't
> > decode the address from an arbitrary pointer, so this would require
> > something different.
>
> So I had a bit of a peek at what clang generates:
>
>     3fa4:       48 c7 c7 00 00 00 00    mov    $0x0,%rdi        3fa7: R_X86_64_32S      __SCK__x86_pmu_handle_irq
>     3fab:       48 c7 c6 00 00 00 00    mov    $0x0,%rsi        3fae: R_X86_64_32S      __SCT__x86_pmu_handle_irq.cfi_jt
>     3fb2:       e8 00 00 00 00          call   3fb7 <init_hw_perf_events+0x1dc> 3fb3: R_X86_64_PLT32    __static_call_update-0x4
>
> So this then gives the trampoline jump table entry to
> __static_call_update(), with the result that it will rewrite the
> jump-table entry, not the trampoline!
>
> Now it so happens that the trampoline looks *exactly* like the
> jump-table entry (one jmp.d32 instruction), so in that regards it'll
> again 'work'.

Ugh, good catch!

> But this is all really, as in *really*, wrong. And I'm really sad I'm
> the one to have to discover this, even though I've mentioned
> static_call()s being tricky in previous reviews.

My bad, I didn't realize Clang does this with a typeof(func)
declaration. I'll make sure we have a reasonable fix for this before
the next version.

Sami