Received-SPF: pass (google.com: domain of linux-kernel+bounces-21359-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
Precedence: bulk
MIME-Version: 1.0
References: <20231213134303.2302285-1-dimitri.ledkov@canonical.com>
 <20231213134303.2302285-2-dimitri.ledkov@canonical.com> <ZZu9Nvkp3PdSeLHQ@gmail.com>
 <20240109192447.yhl37mwaw5jdkxjs@treble>
In-Reply-To: <20240109192447.yhl37mwaw5jdkxjs@treble>
From: Dimitri John Ledkov <dimitri.ledkov@canonical.com>
Date: Tue, 9 Jan 2024 19:48:30 +0000
Message-ID: <CADWks+YO7zdFoYfLn+qwD8FyZoU1EfYCq1sghFkrn-nXANYMhw@mail.gmail.com>
Subject: Re: [PATCH v2 1/3] objtool: Make objtool check actually fatal upon
 fatal errors
To: Josh Poimboeuf <jpoimboe@kernel.org>
Cc: Ingo Molnar <mingo@kernel.org>, peterz@infradead.org, x86@kernel.org, 
	linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"

Hi,

On Tue, 9 Jan 2024 at 19:24, Josh Poimboeuf <jpoimboe@kernel.org> wrote:
>
> On Mon, Jan 08, 2024 at 10:15:34AM +0100, Ingo Molnar wrote:
> >
> > * Dimitri John Ledkov <dimitri.ledkov@canonical.com> wrote:
> >
> > > Currently function calls within check() are sensitive to fatal errors
> > > (negative return codes) and abort execution prematurely. However, in
> > > all such cases the check() function still returns 0, and thus
> > > resulting in a successful kernel build.
> > >
> > > The only correct code paths were the ones that escpae the control flow
> > > with `return ret`.
> > >
> > > Make the check() function return `ret` status code, and make all
> > > negative return codes goto that instruction. This makes fatal errors
> > > (not warnings) from various function calls actually fail the
> > > build. E.g. if create_retpoline_sites_sections() fails to create elf
> > > section pair retpoline_sites the tool now exits with an error code.
> > >
> > > Signed-off-by: Dimitri John Ledkov <dimitri.ledkov@canonical.com>
> >
> > So, is this not expected to be the case anymore:
> >
> > >  out:
> > > -   /*
> > > -    *  For now, don't fail the kernel build on fatal warnings.  These
> > > -    *  errors are still fairly common due to the growing matrix of
> > > -    *  supported toolchains and their recent pace of change.
> > > -    */
> > > -   return 0;
> >
> > ?
> >
> > How about making it only fatal if CONFIG_WERROR=y, ie. an analogue to our
> > treatment of compiler warnings?
>
> Objtool has two classes of warnings:
>
> 1) "fatal"
>
>    - allocation failures
>    - CFG recreation failures
>    - annotation parsing errors
>    - other objtool bugs
>
> 2) non-"fatal":
>
>   - missing security features (retpolines, IBT, SLS INT3)
>   - unreachable instructions (note this warning may indicate more
>     serious issues like an incomplete or buggy objtool CFG)
>
> The first class of "warning" is actually an error.  It means objtool
> couldn't reasonably continue, so it exited early.  I'm thinking this
> should always fail the build so it can be reported and fixed ASAP.
>
> We tried doing that before, but ending up reverting it (for raisins).
> We should try again (as per the above patch).
>
> The second class of warning, though it doesn't abort objtool, can still
> be quite serious.  Ignoring it can fail the boot, or can expose the user
> to certain attacks.
>

As a distribution kernel maintainer, my primary motivation was
enforcement of the security features.
Specifically because these security features are config opt-in, and
when one opts into having them, they better be enfoced.
If something fails, well, you cannot claim that one has retpoline, sls
mitigrations enabled.

> My proposal would be to always fail for #1, and to make #2 dependent on
> CONFIG_WERROR.

I worry about bundling unrelated things. I still want to enforce
security relevant features separately from compiler changes.

How are you proposing to deal with toolchain upgrades? A given kernel
might be WERROR clean with gcc-11 and gcc-12 -march=generic, but not
with -march=x86-64-v3 (as recently found) or for example with
clang-snapshot.

Whilst at the same time, the security relevant things should be enforced.

From a distribution maintainer perspective, if a kernel is compiled
with CONFIG_SLS I want to be able to enforce that for both kernel and
dkms builds. I cannot reasonably keep up with toolchain updates and
dkms code quality w.r.t. WERROR beyond what is required for security
sensitive code paths like SLS and RETPOLINE.

>
> Note the latter may be problematic at the moment due to some outstanding
> warnings reported by Arnd and randconfig build bots.  I try to fix those
> when I can, but any help would be appreciated.

I guess we can override CONFIG_WERROR during dkms compilations, but
then i still want to ensofrce OBJTOOL errors.

How abou introducing CONFIG_WERROR_OBJTOOL and make CONFIG_WERROR
select CONFIG_WERROR_OBJTOOL? such that distributions can start to opt
into CONFIG_WERROR_OBJTOOL for production builds, even if they cannot
yet make the jump to CONFIG_WERROR?

-- 
Dimitri

Sent from Ubuntu Pro
https://ubuntu.com/pro