Date:   Fri, 11 Feb 2022 10:34:09 +0000
From:   Robin Murphy <robin.murphy@....com>
To:     Kees Cook <keescook@...omium.org>, Ard Biesheuvel <ardb@...nel.org>
Cc:     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

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?

> 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.
> 
> 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...

Thanks,
Robin.

> 
> -Kees
> 
> [1] https://github.com/llvm/llvm-project/issues/44261
> 
>>
>>
>>> ---
>>>   drivers/acpi/arm64/iort.c | 12 ++++++------
>>>   1 file changed, 6 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/drivers/acpi/arm64/iort.c b/drivers/acpi/arm64/iort.c
>>> index 3b23fb775ac4..65395f0decf9 100644
>>> --- a/drivers/acpi/arm64/iort.c
>>> +++ b/drivers/acpi/arm64/iort.c
>>> @@ -1645,7 +1645,7 @@ void __init acpi_iort_init(void)
>>>    */
>>>   phys_addr_t __init acpi_iort_dma_get_max_cpu_address(void)
>>>   {
>>> -       phys_addr_t limit = PHYS_ADDR_MAX;
>>> +       phys_addr_t local_limit, limit = PHYS_ADDR_MAX;
>>>          struct acpi_iort_node *node, *end;
>>>          struct acpi_table_iort *iort;
>>>          acpi_status status;
>>> @@ -1667,17 +1667,16 @@ phys_addr_t __init acpi_iort_dma_get_max_cpu_address(void)
>>>                          break;
>>>
>>>                  switch (node->type) {
>>> +               case ACPI_IORT_NODE_NAMED_COMPONENT: {
>>>                          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;
>>>                          local_limit = DMA_BIT_MASK(ncomp->memory_address_limit);
>>>                          limit = min_not_zero(limit, local_limit);
>>>                          break;
>>>
>>> -               case ACPI_IORT_NODE_PCI_ROOT_COMPLEX:
>>> +               }
>>> +               case ACPI_IORT_NODE_PCI_ROOT_COMPLEX: {
>>> +                       struct acpi_iort_root_complex *rc;
>>>                          if (node->revision < 1)
>>>                                  break;
>>>
>>> @@ -1686,6 +1685,7 @@ phys_addr_t __init acpi_iort_dma_get_max_cpu_address(void)
>>>                          limit = min_not_zero(limit, local_limit);
>>>                          break;
>>>                  }
>>> +               }
>>>                  node = ACPI_ADD_PTR(struct acpi_iort_node, node, node->length);
>>>          }
>>>          acpi_put_table(&iort->header);
>>>
>>> _______________________________________________
>>> linux-arm-kernel mailing list
>>> linux-arm-kernel@...ts.infradead.org
>>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> 

Powered by blists - more mailing lists