[PATCH v2] ACPI/IORT: Fix GCC 12 warning
Robin Murphy
robin.murphy at arm.com
Fri Feb 11 02:34:09 PST 2022
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 at oracle.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 at oracle.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 at lists.infradead.org
>>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>
More information about the linux-arm-kernel
mailing list