Message-ID: <202202111623.A7881CC@keescook>
Date:   Fri, 11 Feb 2022 16:37:48 -0800
From:   Kees Cook <keescook@...omium.org>
To:     Robin Murphy <robin.murphy@....com>
Cc:     Ard Biesheuvel <ardb@...nel.org>,
        Victor Erminpour <victor.erminpour@...cle.com>,
        Lorenzo Pieralisi <lorenzo.pieralisi@....com>,
        Hanjun Guo <guohanjun@...wei.com>,
        Sudeep Holla <sudeep.holla@....com>,
        "Rafael J. Wysocki" <rafael@...nel.org>,
        Len Brown <lenb@...nel.org>,
        ACPI Devel Maling List <linux-acpi@...r.kernel.org>,
        Linux ARM <linux-arm-kernel@...ts.infradead.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        trivial@...nel.org
Subject: Re: [PATCH v2] ACPI/IORT: Fix GCC 12 warning

On Fri, Feb 11, 2022 at 10:34:09AM +0000, Robin Murphy wrote:
> Hi Kees,
> 
> On 2022-02-10 23:47, Kees Cook wrote:
> > On Thu, Feb 10, 2022 at 08:41:51PM +0100, Ard Biesheuvel wrote:
> > > On Thu, 10 Feb 2022 at 19:48, Victor Erminpour
> > > <victor.erminpour@...cle.com> wrote:
> > > > 
> > > > When building with automatic stack variable initialization, GCC 12
> > > > complains about variables defined outside of switch case statements.
> > > > Move the variable into the case that uses it, which silences the warning:
> > > > 
> > > > ./drivers/acpi/arm64/iort.c:1670:59: error: statement will never be executed [-Werror=switch-unreachable]
> > > >    1670 |                         struct acpi_iort_named_component *ncomp;
> > > >         |                                                           ^~~~~
> > > > 
> > > > Signed-off-by: Victor Erminpour <victor.erminpour@...cle.com>
> > > 
> > > Please cc people that commented on your v1 when you send a v2.
> > > 
> > > Still NAK, for the same reasons.
> > 
> > Let me see if I can talk you out of this. ;)
> > 
> > So, on the face of it, I agree with you: this is a compiler bug. However,
> > it's still worth fixing. Just because it's valid C isn't a good enough
> > reason to leave it as-is: we continue to minimize the subset of the
> > C language the kernel uses if it helps us get the most out of existing
> > compiler features. We've eliminated all kinds of other "valid C" from the
> > kernel because it improves robustness, security, etc. This is certainly
> > nothing like removing VLAs or implicit fallthrough, but given that this
> > is, I think, the only remaining case of it (I removed all the others a
> > while ago when I had the same issues with the GCC plugins), I'd like to
> > get it fixed.
> 
> It concerns me if minimising the subset of the C language that the kernel
> uses is achieved by converting more of the kernel to a not-quite-C language
> that is not formally specified anywhere, by prematurely adopting
> newly-invented compiler options that clearly don't work properly (the GCC
> warning message quoted above may as well be "error: giraffes are not purple"
> for all the sense it makes.)

Yeah, you're right. While it's a corner case, it's still important to
get it fixed because it risks eroding people's good will for future work.
What you (and Ard) bring up is just as important a roadblock as any of
the other (many *sob*) roadblocks that have been overcome for its
adoption.

> From your security standpoint (and believe me, I really do have faith in
> your expertise here), which of these sounds better:
> 
> 1: Being able to audit code based on well-defined language semantics
> 
> 2: Playing whack-a-mole as issues are discovered empirically.
> 
> 3: Neither of the above, but a warm fuzzy feeling because hey someone said
> "security" in a commit message.
> 
> AFAICS you're effectively voting against #1, and the examples you've given
> demonstrate that #2 is nowhere near reliable enough either, so where does
> that leave us WRT actual secure and robust code in Linux?

Well, I'm for #1, though perhaps with a more narrow view: some semantics
are just weird/surprising. ;) Until I first encountered this warning a
few years ago when working on GCC_PLUGIN_STRUCTLEAK_BYREF_ALL, I didn't
even know putting declarations there was valid C. ;)

Whack-a-mole is part of the work to make these kinds of treewide
changes, but the hope is to find as much of it ahead of time as
possible. And, no, I have no interest in security theater. (Not
everything has equal levels of effectiveness, of course, but I don't
think that's what you're saying.)

> In fairness I'd have no objection to that patch if it came with a convincing
> justification, but that is so far very much lacking. My aim here is not to
> be a change-averse Luddite, but to try to find a compromise where I can
> actually have some confidence in such changes being made. Let's not start
> pretending that 3 100ml bottles of shampoo are somehow "safer" than a 300ml
> bottle of shampoo...

Sure. I think I am trying to take a pragmatic approach here, which is
that gaining auto-var-init is a big deal (killing entire classes of
vulnerabilities), but it comes with an annoying compiler bug (that we do
get a warning about) for an uncommon code pattern that is easy to fix.
So rather than delaying the defense until the sharp edge on the compiler
gets fixed, I'd like to get the rest rolling while the edge is filed.

-Kees

-- 
Kees Cook

Powered by blists - more mailing lists