Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:18 as permitted sender) client-ip=2620:137:e000::1:18;
Date:   Tue, 8 Feb 2022 15:42:33 -0800
From:   Kees Cook <keescook@chromium.org>
To:     joao@overdrivepizza.com
Cc:     Peter Zijlstra <peterz@infradead.org>, x86@kernel.org,
        hjl.tools@gmail.com, jpoimboe@redhat.com,
        andrew.cooper3@citrix.com, linux-kernel@vger.kernel.org,
        ndesaulniers@google.com, samitolvanen@google.com
Subject: Re: [RFC][PATCH 6/6] objtool: Add IBT validation / fixups
Message-ID: <202202081541.900F9E1B@keescook>
References: <20211122170301.764232470@infradead.org>
 <20211122170805.338489412@infradead.org>
 <6ebb0ab131c522f20c094294d49091fc@overdrivepizza.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <6ebb0ab131c522f20c094294d49091fc@overdrivepizza.com>
Precedence: bulk

On Thu, Dec 23, 2021 at 06:05:50PM -0800, joao@overdrivepizza.com wrote:
> 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

Ah nice; I see this has been committed now.

Given that IBT will need to work with both Clang and gcc, I suspect the
objtool approach will still end up needing to do all the verification.

(And as you say, it has limited visibility into assembly.)

-Kees

-- 
Kees Cook