Message-ID: <CAMj1kXHGjTt8xh9WtpA+Gv_cEtA6SeDAPOU0fHoZ8gv2qSXB4A@mail.gmail.com>
Date:   Fri, 11 Feb 2022 18:08:58 +0100
From:   Ard Biesheuvel <ardb@...nel.org>
To:     Robin Murphy <robin.murphy@....com>
Cc:     Kees Cook <keescook@...omium.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, 11 Feb 2022 at 13:16, Robin Murphy <robin.murphy@....com> wrote:
>
> On 2022-02-11 11:43, Ard Biesheuvel wrote:
> > On Fri, 11 Feb 2022 at 11:34, Robin Murphy <robin.murphy@....com> 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.)
> >>
> >>> And I should point out that Clang suffers[1] from the same problem (the
> >>> variables will be missed for auto-initialization), but actually has a
> >>> worse behavior: it does not even warn about it.
> >>>
> >>> And note that the problem isn't limited to -ftrivial-auto-var-init. This
> >>> code pattern seems to also hide the variables from similar instrumentation
> >>> like KASan, etc. (Which is similarly silent like above.)
> >>
> >>   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?
> >>
> >
> > My concerns are more about:
> > - The GCC version of the feature not being fully baked yet, which
> > makes it hard to have full confidence in its implementation (surely,
> > GCC has a test case or two with a switch scope variable declaration;
> > - We waste the credit we have with other developers who care less
> > about security on things that we could have fixed before they'd even
> > notice. What will happen the next time around when we *really* need
> > source level changes?
> >
> >>> In both compilers, it seems fixing this is not "easy", and given its
> >>> corner-case nature and ease of being worked around in the kernel source,
> >>> it isn't being highly prioritized. But since I both don't want these
> >>> blinds spots with Clang (and GCC) var-init, and I don't want these
> >>> warnings to suddenly appear once GCC 12 _does_ get released, so I'd like
> >>> to get this case fixed as well.
> >>>
> >
> > So how is this
> >
> > switch {
> > var foo;
> > case x:
> >     ...
> > }
> >
> > fundamentally different from
> >
> > {
> > var foo;
> > switch {
> > case x:
> >     ...
> > }
> > }
> >
> > Surely, some kind of transformation is possible where the var
> > declaration is hoisted into a parent scope added around the entire
> > switch {} statement?
> >
> >>> All that said, I think this patch could be improved.
> >>>
> >>> I'd recommend, instead, just simply:
> >>>
> >>> diff --git a/drivers/acpi/arm64/iort.c b/drivers/acpi/arm64/iort.c
> >>> index f2f8f05662de..9e765d30da82 100644
> >>> --- a/drivers/acpi/arm64/iort.c
> >>> +++ b/drivers/acpi/arm64/iort.c
> >>> @@ -1671,13 +1671,14 @@ phys_addr_t __init acpi_iort_dma_get_max_cpu_address(void)
> >>>        end = ACPI_ADD_PTR(struct acpi_iort_node, iort, iort->header.length);
> >>>
> >>>        for (i = 0; i < iort->node_count; i++) {
> >>> +             struct acpi_iort_named_component *ncomp;
> >>> +             struct acpi_iort_root_complex *rc;
> >>> +             phys_addr_t local_limit;
> >>> +
> >>>                if (node >= end)
> >>>                        break;
> >>>
> >>>                switch (node->type) {
> >>> -                     struct acpi_iort_named_component *ncomp;
> >>> -                     struct acpi_iort_root_complex *rc;
> >>> -                     phys_addr_t local_limit;
> >>>
> >>>                case ACPI_IORT_NODE_NAMED_COMPONENT:
> >>>                        ncomp = (struct acpi_iort_named_component *)node->node_data;
> >>>
> >>> This results in no change in binary instruction output (when there is no
> >>> auto-init).
> >>
> >> 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...
> >>
> >
> > Not sure I get the shampoo reference, but I just don't think this
> > idiom meets the bar for code that really needs modification for the
> > compiler to be able to do the right thing.
>
> I was alluding to the same concern that you have - wasting developers'
> time and goodwill with churn that lacks solid justification. For me the
> security theatre of international air travel over the last decade has
> successfully outweighed any desire to ever go to an airport again, and
> I'd rather not be driven to take a similar attitude towards security
> patches.
>

Ah yes, of course - I'm a bit slow today.

In any case, I agree that merging this patch wouldn't be the end of
the world, as long as we still fix the compiler. And the NAK on v2 was
just because I got annoyed that the author sent a v2 without cc'ing
the people that were assuming v1 was still under discussion.

Powered by blists - more mailing lists