Message-ID: <6411bc92-1689-e5ef-a270-fe7a29e2b8ba@arm.com>
Date:   Fri, 11 Feb 2022 17:11:53 +0000
From:   Robin Murphy <robin.murphy@....com>
To:     Ard Biesheuvel <ardb@...nel.org>
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 2022-02-11 17:08, Ard Biesheuvel wrote:
> 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.

Ack to that. FWIW I spoke to my toolchain colleagues and there's now a 
GCC bug for this: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=104504

Cheers,
Robin.

Powered by blists - more mailing lists