linux-pci.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Sami Tolvanen <samitolvanen@google.com>
To: Rasmus Villemoes <linux@rasmusvillemoes.dk>
Cc: Kees Cook <keescook@chromium.org>,
	Nathan Chancellor <nathan@kernel.org>,
	Nick Desaulniers <ndesaulniers@google.com>,
	Masahiro Yamada <masahiroy@kernel.org>,
	Will Deacon <will@kernel.org>, Jessica Yu <jeyu@kernel.org>,
	Arnd Bergmann <arnd@arndb.de>, Tejun Heo <tj@kernel.org>,
	"Paul E. McKenney" <paulmck@kernel.org>,
	Christoph Hellwig <hch@infradead.org>,
	Peter Zijlstra <peterz@infradead.org>, bpf <bpf@vger.kernel.org>,
	linux-hardening@vger.kernel.org,
	linux-arch <linux-arch@vger.kernel.org>,
	linux-arm-kernel <linux-arm-kernel@lists.infradead.org>,
	linux-kbuild <linux-kbuild@vger.kernel.org>,
	PCI <linux-pci@vger.kernel.org>,
	LKML <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v3 02/17] cfi: add __cficanonical
Date: Wed, 24 Mar 2021 09:38:45 -0700	[thread overview]
Message-ID: <CABCJKudTZang_aUCnO63MFUc5mud3DKpHUgRFB-e04L__j_XHA@mail.gmail.com> (raw)
In-Reply-To: <92afcbea-1415-2df1-5e78-4e9a7a4d364b@rasmusvillemoes.dk>

On Wed, Mar 24, 2021 at 8:31 AM Rasmus Villemoes
<linux@rasmusvillemoes.dk> wrote:
>
> On 23/03/2021 21.39, Sami Tolvanen wrote:
> > With CONFIG_CFI_CLANG, the compiler replaces a function address taken
> > in C code with the address of a local jump table entry, which passes
> > runtime indirect call checks. However, the compiler won't replace
> > addresses taken in assembly code, which will result in a CFI failure
> > if we later jump to such an address in instrumented C code. The code
> > generated for the non-canonical jump table looks this:
> >
> >   <noncanonical.cfi_jt>: /* In C, &noncanonical points here */
> >       jmp noncanonical
> >   ...
> >   <noncanonical>:        /* function body */
> >       ...
> >
> > This change adds the __cficanonical attribute, which tells the
> > compiler to use a canonical jump table for the function instead. This
> > means the compiler will rename the actual function to <function>.cfi
> > and points the original symbol to the jump table entry instead:
> >
> >   <canonical>:           /* jump table entry */
> >       jmp canonical.cfi
> >   ...
> >   <canonical.cfi>:       /* function body */
> >       ...
> >
> > As a result, the address taken in assembly, or other non-instrumented
> > code always points to the jump table and therefore, can be used for
> > indirect calls in instrumented code without tripping CFI checks.
>
> Random ramblings, I'm trying to understand how this CFI stuff works.
>
> First, patch 1 and 2 explain the pros and cons of canonical vs
> non-canonical jump tables, in either case, there's problems with stuff
> implemented in assembly. But I don't understand why those pros and cons
> then end up with using the non-canonical jump tables by default. IIUC,
> with canonical jump tables, function pointer equality would keep working
> for functions implemented in C, because &func would always refer to the
> same stub "function" that lives in the same object file as func.cfi,
> whereas with the non-canonical version, each TU (or maybe DSO) that
> takes the address of func ends up with its own func.cfi_jt.

Correct.

> There are of course lots of direct calls of assembly functions, but
> I don't think we take the address of such functions very often. So why
> can't we instead equip the declarations of those with a
> __cfi_noncanonical attribute?

Clang doesn't support these attributes in function declarations,
unfortunately. If it did, that would certainly help, until someone
wants to compare addresses of assembly functions, in which case we
would again have a problem.

Another way to work around the issue with canonical CFI would be to
add C wrappers for all address-taken assembly functions, but that's
not quite ideal either. I think most indirect calls to assembly
functions happen in the crypto code, which would have required so many
changes that we decided to default to non-canonical CFI instead. This
resulted in far fewer kernel changes despite the cross-module function
address equality issue.

Sami

  reply	other threads:[~2021-03-24 16:40 UTC|newest]

Thread overview: 35+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-03-23 20:39 [PATCH v3 00/17] Add support for Clang CFI Sami Tolvanen
2021-03-23 20:39 ` [PATCH v3 01/17] add " Sami Tolvanen
2021-03-23 20:39 ` [PATCH v3 02/17] cfi: add __cficanonical Sami Tolvanen
2021-03-24 15:31   ` Rasmus Villemoes
2021-03-24 16:38     ` Sami Tolvanen [this message]
2021-03-23 20:39 ` [PATCH v3 03/17] mm: add generic __va_function and __pa_function macros Sami Tolvanen
2021-03-24  7:13   ` Christoph Hellwig
2021-03-24 15:54     ` Sami Tolvanen
2021-03-25 10:16       ` Mark Rutland
2021-03-25 23:17         ` Sami Tolvanen
2021-03-23 20:39 ` [PATCH v3 04/17] module: ensure __cfi_check alignment Sami Tolvanen
2021-03-26  4:34   ` Kees Cook
2021-03-29  9:26   ` Jessica Yu
2021-03-23 20:39 ` [PATCH v3 05/17] workqueue: use WARN_ON_FUNCTION_MISMATCH Sami Tolvanen
2021-03-26  4:34   ` Kees Cook
2021-03-23 20:39 ` [PATCH v3 06/17] kthread: " Sami Tolvanen
2021-03-26  4:35   ` Kees Cook
2021-03-23 20:39 ` [PATCH v3 07/17] kallsyms: strip ThinLTO hashes from static functions Sami Tolvanen
2021-03-23 20:39 ` [PATCH v3 08/17] bpf: disable CFI in dispatcher functions Sami Tolvanen
2021-03-23 20:39 ` [PATCH v3 09/17] treewide: Change list_sort to use const pointers Sami Tolvanen
2021-03-23 21:28   ` Nick Desaulniers
2021-03-24  7:10   ` Christoph Hellwig
2021-03-26  4:35   ` Kees Cook
2021-03-23 20:39 ` [PATCH v3 10/17] lkdtm: use __va_function Sami Tolvanen
2021-03-23 20:39 ` [PATCH v3 11/17] psci: use __pa_function for cpu_resume Sami Tolvanen
2021-03-25 10:23   ` Mark Rutland
2021-03-23 20:39 ` [PATCH v3 12/17] arm64: implement __va_function Sami Tolvanen
2021-03-25 10:37   ` Mark Rutland
2021-03-25 23:27     ` Sami Tolvanen
2021-03-26  0:03       ` Peter Collingbourne
2021-03-23 20:39 ` [PATCH v3 13/17] arm64: use __pa_function Sami Tolvanen
2021-03-23 20:39 ` [PATCH v3 14/17] arm64: add __nocfi to functions that jump to a physical address Sami Tolvanen
2021-03-23 20:39 ` [PATCH v3 15/17] arm64: add __nocfi to __apply_alternatives Sami Tolvanen
2021-03-23 20:39 ` [PATCH v3 16/17] KVM: arm64: Disable CFI for nVHE Sami Tolvanen
2021-03-23 20:39 ` [PATCH v3 17/17] arm64: allow CONFIG_CFI_CLANG to be selected Sami Tolvanen

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=CABCJKudTZang_aUCnO63MFUc5mud3DKpHUgRFB-e04L__j_XHA@mail.gmail.com \
    --to=samitolvanen@google.com \
    --cc=arnd@arndb.de \
    --cc=bpf@vger.kernel.org \
    --cc=hch@infradead.org \
    --cc=jeyu@kernel.org \
    --cc=keescook@chromium.org \
    --cc=linux-arch@vger.kernel.org \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-hardening@vger.kernel.org \
    --cc=linux-kbuild@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-pci@vger.kernel.org \
    --cc=linux@rasmusvillemoes.dk \
    --cc=masahiroy@kernel.org \
    --cc=nathan@kernel.org \
    --cc=ndesaulniers@google.com \
    --cc=paulmck@kernel.org \
    --cc=peterz@infradead.org \
    --cc=tj@kernel.org \
    --cc=will@kernel.org \
    --subject='Re: [PATCH v3 02/17] cfi: add __cficanonical' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).