[PATCH v2] ACPI/IORT: Fix GCC 12 warning

[PATCH v2] ACPI/IORT: Fix GCC 12 warning

[Victor Erminpour]

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@oracle.com>
---
 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);

Re: [PATCH v2] ACPI/IORT: Fix GCC 12 warning

[Ard Biesheuvel]

On Thu, 10 Feb 2022 at 19:48, Victor Erminpour
<victor.erminpour@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@oracle.com>

Please cc people that commented on your v1 when you send a v2.

Still NAK, for the same reasons.


> ---
>  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@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2] ACPI/IORT: Fix GCC 12 warning

[Kees Cook]

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

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

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

-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@lists.infradead.org
> > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

-- 
Kees Cook

Re: [PATCH v2] ACPI/IORT: Fix GCC 12 warning

[Robin Murphy]

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@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@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@lists.infradead.org
>>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> 

Re: [PATCH v2] ACPI/IORT: Fix GCC 12 warning

[Ard Biesheuvel]

On Fri, 11 Feb 2022 at 11:34, Robin Murphy <robin.murphy@arm.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@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@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?
>

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.


> >
> > [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@lists.infradead.org
> >>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> >

Re: [PATCH v2] ACPI/IORT: Fix GCC 12 warning

[Robin Murphy]

On 2022-02-11 11:43, Ard Biesheuvel wrote:
> On Fri, 11 Feb 2022 at 11:34, Robin Murphy <robin.murphy@arm.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@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@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?
>>
> 
> 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.

Cheers,
Robin.

Re: [PATCH v2] ACPI/IORT: Fix GCC 12 warning

[Ard Biesheuvel]

On Fri, 11 Feb 2022 at 13:16, Robin Murphy <robin.murphy@arm.com> wrote:
>
> On 2022-02-11 11:43, Ard Biesheuvel wrote:
> > On Fri, 11 Feb 2022 at 11:34, Robin Murphy <robin.murphy@arm.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@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@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?
> >>
> >
> > 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.

Re: [PATCH v2] ACPI/IORT: Fix GCC 12 warning

[Robin Murphy]

On 2022-02-11 17:08, Ard Biesheuvel wrote:
> On Fri, 11 Feb 2022 at 13:16, Robin Murphy <robin.murphy@arm.com> wrote:
>>
>> On 2022-02-11 11:43, Ard Biesheuvel wrote:
>>> On Fri, 11 Feb 2022 at 11:34, Robin Murphy <robin.murphy@arm.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@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@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?
>>>>
>>>
>>> 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.

Re: [PATCH v2] ACPI/IORT: Fix GCC 12 warning

[Kees Cook]

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

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

Re: [PATCH v2] ACPI/IORT: Fix GCC 12 warning

[Joe Perches]

On Fri, 2022-02-11 at 16:37 -0800, Kees Cook wrote:
[]
> 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.

coccinelle would probably find most all of them.

$ cat switch_define.cocci
@@
expression e;
type t;
identifier i;
@@

	switch (e) {
*	t i;
	}
	
$ spatch --very-quiet -sp-file switch_define.cocci .
13831 files match
diff -u -p ./arch/arc/kernel/unwind.c /tmp/nothing/arch/arc/kernel/unwind.c
--- ./arch/arc/kernel/unwind.c
+++ /tmp/nothing/arch/arc/kernel/unwind.c
@@ -718,7 +718,6 @@ static int processCFI(const u8 *start, c
 	}
 	for (ptr.p8 = start; result && ptr.p8 < end;) {
 		switch (*ptr.p8 >> 6) {
-			uleb128_t value;
 
 		case 0:
 			opcode = *ptr.p8++;
diff -u -p ./arch/arm/kernel/module-plts.c /tmp/nothing/arch/arm/kernel/module-plts.c
--- ./arch/arm/kernel/module-plts.c
+++ /tmp/nothing/arch/arm/kernel/module-plts.c
@@ -124,7 +124,6 @@ static bool is_zero_addend_relocation(El
 	 * PC bias into account, i.e., -8 for ARM and -4 for Thumb2.
 	 */
 	switch (ELF32_R_TYPE(rel->r_info)) {
-		u16 upper, lower;
 
 	case R_ARM_THM_CALL:
 	case R_ARM_THM_JUMP24:
diff -u -p ./arch/mips/mti-malta/malta-init.c /tmp/nothing/arch/mips/mti-malta/malta-init.c
--- ./arch/mips/mti-malta/malta-init.c
+++ /tmp/nothing/arch/mips/mti-malta/malta-init.c
@@ -168,7 +168,6 @@ void __init prom_init(void)
 	}
 
 	switch (mips_revision_sconid) {
-		u32 start, map, mask, data;
 
 	case MIPS_REVISION_SCON_GT64120:
 		/*
diff -u -p ./arch/parisc/kernel/inventory.c /tmp/nothing/arch/parisc/kernel/inventory.c
--- ./arch/parisc/kernel/inventory.c
+++ /tmp/nothing/arch/parisc/kernel/inventory.c
@@ -235,7 +235,6 @@ pat_query_module(ulong pcell_loc, ulong
 #ifdef DEBUG_PAT
 	/* dump what we see so far... */
 	switch (PAT_GET_ENTITY(dev->mod_info)) {
-		pdc_pat_cell_mod_maddr_block_t io_pdc_cell;
 		unsigned long i;
 
 	case PAT_ENTITY_PROC:
diff -u -p ./arch/powerpc/xmon/xmon.c /tmp/nothing/arch/powerpc/xmon/xmon.c
--- ./arch/powerpc/xmon/xmon.c
+++ /tmp/nothing/arch/powerpc/xmon/xmon.c
@@ -1528,7 +1528,6 @@ bpt_cmds(void)
 	cmd = inchar();
 
 	switch (cmd) {
-	static const char badaddr[] = "Only kernel addresses are permitted for breakpoints\n";
 	int mode;
 	case 'd':	/* bd - hardware data breakpoint */
 		if (xmon_is_ro) {
diff -u -p ./arch/um/drivers/ubd_kern.c /tmp/nothing/arch/um/drivers/ubd_kern.c
--- ./arch/um/drivers/ubd_kern.c
+++ /tmp/nothing/arch/um/drivers/ubd_kern.c
@@ -1407,7 +1407,6 @@ static int ubd_ioctl(struct block_device
 	u16 ubd_id[ATA_ID_WORDS];
 
 	switch (cmd) {
-		struct cdrom_volctrl volume;
 	case HDIO_GET_IDENTITY:
 		memset(&ubd_id, 0, ATA_ID_WORDS * 2);
 		ubd_id[ATA_ID_CYLS]	= ubd_dev->size / (128 * 32 * 512);
diff -u -p ./arch/x86/kernel/cpu/cyrix.c /tmp/nothing/arch/x86/kernel/cpu/cyrix.c
--- ./arch/x86/kernel/cpu/cyrix.c
+++ /tmp/nothing/arch/x86/kernel/cpu/cyrix.c
@@ -224,7 +224,6 @@ static void init_cyrix(struct cpuinfo_x8
 	 */
 
 	switch (dir0_msn) {
-		unsigned char tmp;
 
 	case 0: /* Cx486SLC/DLC/SRx/DRx */
 		p = Cx486_name[dir0_lsn & 7];
diff -u -p ./arch/x86/pci/irq.c /tmp/nothing/arch/x86/pci/irq.c
--- ./arch/x86/pci/irq.c
+++ /tmp/nothing/arch/x86/pci/irq.c
@@ -959,7 +959,6 @@ static __init int intel_router_probe(str
 		return 0;
 
 	switch (device) {
-		u8 rid;
 	case PCI_DEVICE_ID_INTEL_82375:
 		r->name = "PCEB/ESC";
 		r->get = pirq_esc_get;
diff -u -p ./drivers/acpi/arm64/iort.c /tmp/nothing/drivers/acpi/arm64/iort.c
--- ./drivers/acpi/arm64/iort.c
+++ /tmp/nothing/drivers/acpi/arm64/iort.c
@@ -1667,7 +1667,6 @@ phys_addr_t __init acpi_iort_dma_get_max
 			break;
 
 		switch (node->type) {
-			struct acpi_iort_named_component *ncomp;
 			struct acpi_iort_root_complex *rc;
 			phys_addr_t local_limit;
 
diff -u -p ./drivers/infiniband/hw/irdma/hw.c /tmp/nothing/drivers/infiniband/hw/irdma/hw.c
--- ./drivers/infiniband/hw/irdma/hw.c
+++ /tmp/nothing/drivers/infiniband/hw/irdma/hw.c
@@ -267,7 +267,6 @@ static void irdma_process_aeq(struct ird
 		}
 
 		switch (info->ae_id) {
-			struct irdma_cm_node *cm_node;
 		case IRDMA_AE_LLP_CONNECTION_ESTABLISHED:
 			cm_node = iwqp->cm_node;
 			if (cm_node->accept_pend) {
diff -u -p ./drivers/infiniband/hw/irdma/utils.c /tmp/nothing/drivers/infiniband/hw/irdma/utils.c
--- ./drivers/infiniband/hw/irdma/utils.c
+++ /tmp/nothing/drivers/infiniband/hw/irdma/utils.c
@@ -1212,7 +1212,6 @@ enum irdma_status_code irdma_hw_modify_q
 			return status;
 
 		switch (m_info->next_iwarp_state) {
-			struct irdma_gen_ae_info ae_info;
 
 		case IRDMA_QP_STATE_RTS:
 		case IRDMA_QP_STATE_IDLE:
diff -u -p ./drivers/input/serio/hil_mlc.c /tmp/nothing/drivers/input/serio/hil_mlc.c
--- ./drivers/input/serio/hil_mlc.c
+++ /tmp/nothing/drivers/input/serio/hil_mlc.c
@@ -633,7 +633,6 @@ static int hilse_donode(hil_mlc *mlc)
 	node = hil_mlc_se + mlc->seidx;
 
 	switch (node->act) {
-		int rc;
 		hil_packet pack;
 
 	case HILSE_FUNC:
diff -u -p ./drivers/rtc/rtc-pcf8523.c /tmp/nothing/drivers/rtc/rtc-pcf8523.c
--- ./drivers/rtc/rtc-pcf8523.c
+++ /tmp/nothing/drivers/rtc/rtc-pcf8523.c
@@ -242,7 +242,6 @@ static int pcf8523_param_get(struct devi
 	int ret;
 
 	switch(param->param) {
-		u32 value;
 
 	case RTC_PARAM_BACKUP_SWITCH_MODE:
 		ret = regmap_read(pcf8523->regmap, PCF8523_REG_CONTROL3, &value);
@@ -281,7 +280,6 @@ static int pcf8523_param_set(struct devi
 	struct pcf8523 *pcf8523 = dev_get_drvdata(dev);
 
 	switch(param->param) {
-		u8 mode;
 	case RTC_PARAM_BACKUP_SWITCH_MODE:
 		switch (param->uvalue) {
 		case RTC_BSM_DISABLED:
diff -u -p ./drivers/rtc/rtc-rv3028.c /tmp/nothing/drivers/rtc/rtc-rv3028.c
--- ./drivers/rtc/rtc-rv3028.c
+++ /tmp/nothing/drivers/rtc/rtc-rv3028.c
@@ -523,7 +523,6 @@ static int rv3028_param_get(struct devic
 	int ret;
 
 	switch(param->param) {
-		u32 value;
 
 	case RTC_PARAM_BACKUP_SWITCH_MODE:
 		ret = regmap_read(rv3028->regmap, RV3028_BACKUP, &value);
@@ -556,7 +555,6 @@ static int rv3028_param_set(struct devic
 	struct rv3028_data *rv3028 = dev_get_drvdata(dev);
 
 	switch(param->param) {
-		u8 mode;
 	case RTC_PARAM_BACKUP_SWITCH_MODE:
 		switch (param->uvalue) {
 		case RTC_BSM_DISABLED:
diff -u -p ./drivers/rtc/rtc-rv3032.c /tmp/nothing/drivers/rtc/rtc-rv3032.c
--- ./drivers/rtc/rtc-rv3032.c
+++ /tmp/nothing/drivers/rtc/rtc-rv3032.c
@@ -401,7 +401,6 @@ static int rv3032_param_get(struct devic
 	int ret;
 
 	switch(param->param) {
-		u32 value;
 
 	case RTC_PARAM_BACKUP_SWITCH_MODE:
 		ret = regmap_read(rv3032->regmap, RV3032_PMU, &value);
@@ -435,7 +434,6 @@ static int rv3032_param_set(struct devic
 	struct rv3032_data *rv3032 = dev_get_drvdata(dev);
 
 	switch(param->param) {
-		u8 mode;
 	case RTC_PARAM_BACKUP_SWITCH_MODE:
 		if (rv3032->trickle_charger_set)
 			return -EINVAL;
diff -u -p ./drivers/s390/net/ctcm_fsms.c /tmp/nothing/drivers/s390/net/ctcm_fsms.c
--- ./drivers/s390/net/ctcm_fsms.c
+++ /tmp/nothing/drivers/s390/net/ctcm_fsms.c
@@ -1432,7 +1432,6 @@ static void ctcmpc_chx_rx(fsm_instance *
 
 again:
 	switch (fsm_getstate(grp->fsm)) {
-	int rc, dolock;
 	case MPCG_STATE_FLOWC:
 	case MPCG_STATE_READY:
 		if (ctcm_checkalloc_buffer(ch))
diff -u -p ./lib/test_stackinit.c /tmp/nothing/lib/test_stackinit.c
--- ./lib/test_stackinit.c
+++ /tmp/nothing/lib/test_stackinit.c
@@ -398,7 +398,6 @@ static int noinline __leaf_switch_none(i
 		 * This is intentionally unreachable. To silence the
 		 * warning, build with -Wno-switch-unreachable
 		 */
-		uint64_t var;
 
 	case 1:
 		target_start = &var;
diff -u -p ./net/mpls/af_mpls.c /tmp/nothing/net/mpls/af_mpls.c
--- ./net/mpls/af_mpls.c
+++ /tmp/nothing/net/mpls/af_mpls.c
@@ -1621,7 +1621,6 @@ static int mpls_dev_notify(struct notifi
 		return NOTIFY_OK;
 
 	switch (event) {
-		int err;
 
 	case NETDEV_DOWN:
 		err = mpls_ifdown(dev, event);
diff -u -p ./scripts/recordmcount.c /tmp/nothing/scripts/recordmcount.c
--- ./scripts/recordmcount.c
+++ /tmp/nothing/scripts/recordmcount.c
@@ -488,7 +488,6 @@ static int do_file(char const *const fna
 	w2 = w2nat;
 	w8 = w8nat;
 	switch (ehdr->e_ident[EI_DATA]) {
-		static unsigned int const endian = 1;
 	default:
 		fprintf(stderr, "unrecognized ELF data encoding %d: %s\n",
 			ehdr->e_ident[EI_DATA], fname);
diff -u -p ./tools/testing/selftests/powerpc/nx-gzip/gunz_test.c /tmp/nothing/tools/testing/selftests/powerpc/nx-gzip/gunz_test.c
--- ./tools/testing/selftests/powerpc/nx-gzip/gunz_test.c
+++ /tmp/nothing/tools/testing/selftests/powerpc/nx-gzip/gunz_test.c
@@ -805,7 +805,6 @@ ok_cc3:
 	 */
 
 	switch (sfbt) {
-		int dhtlen;
 
 	case 0x0: /* Deflate final EOB received */
 
diff -u -p ./tools/testing/selftests/proc/read.c /tmp/nothing/tools/testing/selftests/proc/read.c
--- ./tools/testing/selftests/proc/read.c
+++ /tmp/nothing/tools/testing/selftests/proc/read.c
@@ -91,7 +91,6 @@ static void f(DIR *d, unsigned int level
 		assert(!streq(de->d_name, ".."));
 
 		switch (de->d_type) {
-			DIR *dd;
 			int fd;
 
 		case DT_REG:
$ 

RE: [PATCH v2] ACPI/IORT: Fix GCC 12 warning

[David Laight]

From: Kees Cook
> Sent: 10 February 2022 23:47
...
> 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;
> +

I'd move them right to the top of the function.
No point hiding the declarations in the middle.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

RE: [PATCH v2] ACPI/IORT: Fix GCC 12 warning

[David Laight]

From: Kees Cook
> Sent: 12 February 2022 00:38
....
> 
> 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.

My worry about the compiler setting variables to zero is that people
will get lazy and assume it happens.
Then some code will get compiled without that enabled (possibly because
it slows things down too much) and then really horrid bugs start to appear.

If the intent is to detect code that is failing to initialise locals
then setting to a non-zero value (that is likely to cause grief)
is much better than setting to zero.

I'm particularly worried if the compiler starts to zero on-stack arrays.
There is plenty of userspace code that will use char buff[MAX_PATH]
to build a filename or char errmsg[1024] for an error message.
At the moment the size only needs to be 'big enough' but if the compiler
zeros it then there are performance issues and oversize buffers make
a difference.
At which point you start making the arrays 'just big enough' and then
fall foul of truncations and overruns because the calculation was wrong.

Definitely double edged.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)