LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: joao@overdrivepizza.com
To: Peter Zijlstra <peterz@infradead.org>
Cc: x86@kernel.org, hjl.tools@gmail.com, jpoimboe@redhat.com,
	andrew.cooper3@citrix.com, linux-kernel@vger.kernel.org,
	ndesaulniers@google.com, keescook@chromium.org,
	samitolvanen@google.com
Subject: Re: [RFC][PATCH 6/6] objtool: Add IBT validation / fixups
Date: Thu, 23 Dec 2021 18:05:50 -0800	[thread overview]
Message-ID: <6ebb0ab131c522f20c094294d49091fc@overdrivepizza.com> (raw)
In-Reply-To: <20211122170805.338489412@infradead.org>

On 2021-11-22 09:03, Peter Zijlstra wrote:
> Objtool based IBT validation in 3 passes:
> 
>  --ibt-fix-direct:
> 
>     Detect and rewrite any code/reloc from a JMP/CALL instruction
>     to an ENDBR instruction. This is basically a compiler bug since
>     neither needs the ENDBR and decoding it is a pure waste of time.
> 
>  --ibt:
> 
>     Report code relocs that are not JMP/CALL and don't point to ENDBR
> 
>     There's a bunch of false positives, for eg. static_call_update()
>     and copy_thread() and kprobes. But most of them were due to
>     _THIS_IP_ which has been taken care of with the prior annotation.
> 
>  --ibt-seal:
> 
>     Find and NOP superfluous ENDBR instructions. Any function that
>     doesn't have it's address taken should not have an ENDBR
>     instruction. This removes about 1-in-4 ENDBR instructions.
> 

I did some experimentation with compiler-based implementation for two of 
the features described here (plus an additional one). Before going into 
details, just a quick highlight that the compiler has limited visibility 
over handwritten assembly sources thus, depending on the feature, a 
compiler-based approach will not cover as much as objtool. All the 
observations below were made when compiling the kernel with defconfig, + 
CLANG-related options, + LTO sometimes. Here I used kernel revision 
0fcfb00b28c0b7884635dacf38e46d60bf3d4eb1 with PeterZ's IBT Beginning 
patches applied on top (plus changes to Kbuild), thus, IBT was not 
really enforced. Tests consisted mostly of Clang's synthetics tests + 
booting a compiled kernel.

Prototypes of the features are available in: 
https://github.com/lvwr/llvm/tree/joao/ibt -- I fixed as many corner 
cases I could find while trying it out, but I believe some might still 
be haunting. Also, I'm not very keen to Kbuild internals nor to however 
the kernel patches itself during runtime, so I may have missed some 
details.

Finally, I'm interested in general feedback about this... better ways of 
implementing, alternative approaches, new possible optimizations and 
everything. I should be AFK for a few days in the next weeks, but I'll 
be glad to discuss this in January and then. Happy Holidays :)

The features:

-mibt-seal:

Add ENDBRs exclusively to address-taken functions regardless of its 
linkage visibility. Only make sense together with LTO.

Observations: Reduced drastically the number of ENDBRs placed in the 
kernel binary (From 44383 to 33192), but still missed some that were 
later fixed by objtool (Number of fixes by objtool reduced from 11730 to 
540). I did not investigate further why these superfluous ENDBRs were 
still left in the binary, but at this point my hypotheses spin around 
(i) false-positive address-taken conclusions by the compiler, possibly 
due to things like exported symbols and such; (ii) assembly sources 
which are invisible to the compiler (although this would more likely 
hide address taken functions); (iii) other binary level transformations 
done by objtool.

Runtime testing: The kernel was verified to properly boot after being 
compiled with -mibt-seal (+ LTO).

Note: This feature was already submitted for upstreaming with the 
llvm-project: https://reviews.llvm.org/D116070

-mibt-fix-direct:

Direct calls to functions which are known to have ENDBR instructions are 
fixed to target the first instruction after the ENDBR (+4 offset). Does 
not necessarily depend on LTO, but is meaningfully improved by it 
because it depends on after-linking stuff to successfully identify 
targets that will have ENDBRs (aliases and assembly functions are a big 
complication here, so this currently won't try to optimize calls to 
functions which exist in the compiler context as declarations).

Observations: I did a quick change on objtool to collect stats on the 
number of fixes applied. Without -mibt-fix-direct objtool had to fix 
227552 direct calls/jmps. Without LTO, -mibt-fix-direct reduced this 
number to 218455. With LTO this number was reduced to 1728.

Runtime testing: The kernel was verified to properly boot when compiled 
with -mibt-fix-direct both with and without LTO. I tried a more 
aggressive approach for non-LTO compilation, which was trying to 
optimize based on declarations (when functions were not visible) and 
noticed that in this scenario the kernel would crash very early in the 
boot process. I did not investigate further, but my hypothesis is that 
this approach mistakenly optimizes calls to assembly functions without 
ENDBRs, leading to random weirdness and doom.

-mibt-preceding-endbr:

Instead of placing ENDBRs after the function entry label, place it right 
before. Indirect calls are fixed (using a sub instr) to target 4 bytes 
before the function entry point. Address values which are reused after 
the indirect call are fixed back to their original value (using an add 
instr). Indirect calls that use in-memory pointers are transformed to 
first load the function pointer from memory into a free register, then 
do the sub, then call it. This does not depend on LTO.

Runtime testing: The runtime test was done using a kernel whose asm 
functions were not fixed regarding the position of ENDBRs. Yet, perhaps 
surprisingly, the kernel still boots when compiled with 
-mibt-preceding-endbr. I don't really know how many indirect calls to 
assembly functions happened, but I assume that these would have crashed 
if IBT was being enforced due to the misplaced ENDBRs (and it could also 
have crashed in these early tests due to indirect calls targeting 
unknown instruction 4 bytes before assembly functions, thus the surprise 
factor when I saw the kernel booting). Kernel booted alright with 
-mibt-preceding-endbr with and without LTO. CONFIG_RETPOLINE was 
disabled for testing this.

  parent reply	other threads:[~2021-12-24  2:05 UTC|newest]

Thread overview: 46+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-11-22 17:03 [RFC][PATCH 0/6] x86: Kernel IBT beginnings Peter Zijlstra
2021-11-22 17:03 ` [RFC][PATCH 1/6] x86: Annotate _THIS_IP_ Peter Zijlstra
2021-11-23 13:53   ` Mark Rutland
2021-11-23 14:14     ` Peter Zijlstra
2021-11-24 18:18       ` Josh Poimboeuf
2021-11-22 17:03 ` [RFC][PATCH 2/6] x86: Base IBT bits Peter Zijlstra
2022-02-08 23:32   ` Kees Cook
2021-11-22 17:03 ` [RFC][PATCH 3/6] x86: Add ENDBR to IRET-to-Self Peter Zijlstra
2021-11-22 18:09   ` Peter Zijlstra
2022-02-08 23:33     ` Kees Cook
2021-11-22 17:03 ` [RFC][PATCH 4/6] objtool: Read the _THIS_IP_ hints Peter Zijlstra
2021-11-22 17:03 ` [RFC][PATCH 5/6] x86: Sprinkle ENDBR dust Peter Zijlstra
2021-11-23 14:00   ` Mark Rutland
2021-11-23 14:21     ` Peter Zijlstra
2022-02-08 23:38     ` Kees Cook
2021-11-22 17:03 ` [RFC][PATCH 6/6] objtool: Add IBT validation / fixups Peter Zijlstra
2021-11-24 19:30   ` Josh Poimboeuf
2022-02-08 23:43     ` Kees Cook
2022-02-09  5:09       ` Josh Poimboeuf
2022-02-09 11:41       ` Peter Zijlstra
2022-02-09 11:45         ` Peter Zijlstra
2021-12-24  2:05   ` joao [this message]
2022-02-08 23:42     ` Kees Cook
2022-02-09  2:21       ` Joao Moreira
2022-02-09  4:05         ` Kees Cook
2022-02-09  5:18           ` Joao Moreira
2022-02-11 13:38             ` Peter Zijlstra
2022-02-14 21:38               ` Sami Tolvanen
2022-02-14 22:25                 ` Peter Zijlstra
2022-02-15 16:56                   ` Sami Tolvanen
2022-02-15 20:03                     ` Kees Cook
2022-02-15 21:05                       ` Peter Zijlstra
2022-02-15 23:05                         ` Kees Cook
2022-02-15 23:38                           ` Joao Moreira
2022-02-16 12:24                         ` Peter Zijlstra
2022-02-15 20:53                     ` Peter Zijlstra
2022-02-15 22:45               ` Joao Moreira
2022-02-16  0:57               ` Andrew Cooper
2022-03-02  3:06               ` Peter Collingbourne
2022-03-02  3:32                 ` Joao Moreira
2022-06-08 17:53                 ` Fāng-ruì Sòng
2022-06-09  0:05                   ` Sami Tolvanen
2021-11-23  7:58 ` [RFC][PATCH 0/6] x86: Kernel IBT beginnings Christoph Hellwig
2021-11-23  9:02   ` Peter Zijlstra
2022-02-08 23:48 ` Kees Cook
2022-02-09  0:09 ` Nick Desaulniers

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=6ebb0ab131c522f20c094294d49091fc@overdrivepizza.com \
    --to=joao@overdrivepizza.com \
    --cc=andrew.cooper3@citrix.com \
    --cc=hjl.tools@gmail.com \
    --cc=jpoimboe@redhat.com \
    --cc=keescook@chromium.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=ndesaulniers@google.com \
    --cc=peterz@infradead.org \
    --cc=samitolvanen@google.com \
    --cc=x86@kernel.org \
    --subject='Re: [RFC][PATCH 6/6] objtool: Add IBT validation / fixups' \
    /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).