[v2] std/tcg: Replace zero-length array with flexible-array member

[SeaBIOS] [PATCH v2] std/tcg: Replace zero-length array with flexible-array member

Posted by Paul Menzel 5 years, 11 months ago

Date: Tue, 3 Mar 2020 16:24:46 +0100

GCC 10 gives the warnings below:

    In file included from out/ccode32flat.o.tmp.c:54:
    ./src/tcgbios.c: In function 'tpm20_write_EfiSpecIdEventStruct':
    ./src/tcgbios.c:290:30: warning: array subscript '(<unknown>) + 4294967295' is outside the bounds of an interior zero-length array 'struct TCG_EfiSpecIdEventAlgorithmSize[0]' [-Wzero-length-bounds]
      290 |         event.hdr.digestSizes[count].algorithmId = be16_to_cpu(sel->hashAlg);
          |         ~~~~~~~~~~~~~~~~~~~~~^~~~~~~
    In file included from ./src/tcgbios.c:22,
                     from out/ccode32flat.o.tmp.c:54:
    ./src/std/tcg.h:527:7: note: while referencing 'digestSizes'
      527 |     } digestSizes[0];
          |       ^~~~~~~~~~~
    In file included from out/ccode32flat.o.tmp.c:54:
    ./src/tcgbios.c:291:30: warning: array subscript '(<unknown>) + 4294967295' is outside the bounds of an interior zero-length array 'struct TCG_EfiSpecIdEventAlgorithmSize[0]' [-Wzero-length-bounds]
      291 |         event.hdr.digestSizes[count].digestSize = hsize;
          |         ~~~~~~~~~~~~~~~~~~~~~^~~~~~~
    In file included from ./src/tcgbios.c:22,
                     from out/ccode32flat.o.tmp.c:54:
    ./src/std/tcg.h:527:7: note: while referencing 'digestSizes'
      527 |     } digestSizes[0];
          |       ^~~~~~~~~~~

[Description copied from Gustavo A. R. Silva <gustavo@embeddedor.com>
from his Linux kernel commits.]

The current codebase makes use of the zero-length array language
extension to the C90 standard, but the preferred mechanism to declare
variable-length types such as these ones is a flexible array
member [1][2], introduced in C99:

    struct foo {
            int stuff;
            struct boo array[];
    };

By making use of the mechanism above, we will get a compiler warning
in case the flexible array does not occur last in the structure, which
will help us prevent some kind of undefined behavior bugs from being
inadvertently introduced[3] to the codebase from now on.

Also, notice that, dynamic memory allocations won't be affected by
this change:

"Flexible array members have incomplete type, and so the sizeof operator
may not be applied. As a quirk of the original implementation of
zero-length arrays, sizeof evaluates to zero."[1]

This issue was found with the help of Coccinelle.

[1] https://gcc.gnu.org/onlinedocs/gcc/Zero-Length.html
[2] https://github.com/KSPP/linux/issues/21
[3] commit 76497732932f ("cxgb3/l2t: Fix undefined behaviour")
    https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=76497732932f15e7323dc805e8ea8dc11bb587cf

Signed-off-by: Paul Menzel <pmenzel@molgen.mpg.de>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
v2: Cite all references, add Reviewed-by line

 src/std/tcg.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/std/tcg.h b/src/std/tcg.h
index 1cc1c92..1c9eeb4 100644
--- a/src/std/tcg.h
+++ b/src/std/tcg.h
@@ -524,7 +524,7 @@ struct TCG_EfiSpecIdEventStruct {
     struct TCG_EfiSpecIdEventAlgorithmSize {
         u16 algorithmId;
         u16 digestSize;
-    } digestSizes[0];
+    } digestSizes[];
     /*
     u8 vendorInfoSize;
     u8 vendorInfo[0];
-- 
2.25.0

_______________________________________________
SeaBIOS mailing list -- seabios@seabios.org
To unsubscribe send an email to seabios-leave@seabios.org

[SeaBIOS] Re: [PATCH v2] std/tcg: Replace zero-length array with flexible-array member

Posted by Kevin O'Connor 5 years, 11 months ago

On Wed, Mar 04, 2020 at 02:51:27PM +0100, Paul Menzel wrote:
> Date: Tue, 3 Mar 2020 16:24:46 +0100
> 
> GCC 10 gives the warnings below:
> 
>     In file included from out/ccode32flat.o.tmp.c:54:
>     ./src/tcgbios.c: In function 'tpm20_write_EfiSpecIdEventStruct':
>     ./src/tcgbios.c:290:30: warning: array subscript '(<unknown>) + 4294967295' is outside the bounds of an interior zero-length array 'struct TCG_EfiSpecIdEventAlgorithmSize[0]' [-Wzero-length-bounds]
>       290 |         event.hdr.digestSizes[count].algorithmId = be16_to_cpu(sel->hashAlg);
>           |         ~~~~~~~~~~~~~~~~~~~~~^~~~~~~

Thanks.  It looks fine to me.  Stefan - do you have any comments on this?

-Kevin



>     In file included from ./src/tcgbios.c:22,
>                      from out/ccode32flat.o.tmp.c:54:
>     ./src/std/tcg.h:527:7: note: while referencing 'digestSizes'
>       527 |     } digestSizes[0];
>           |       ^~~~~~~~~~~
>     In file included from out/ccode32flat.o.tmp.c:54:
>     ./src/tcgbios.c:291:30: warning: array subscript '(<unknown>) + 4294967295' is outside the bounds of an interior zero-length array 'struct TCG_EfiSpecIdEventAlgorithmSize[0]' [-Wzero-length-bounds]
>       291 |         event.hdr.digestSizes[count].digestSize = hsize;
>           |         ~~~~~~~~~~~~~~~~~~~~~^~~~~~~
>     In file included from ./src/tcgbios.c:22,
>                      from out/ccode32flat.o.tmp.c:54:
>     ./src/std/tcg.h:527:7: note: while referencing 'digestSizes'
>       527 |     } digestSizes[0];
>           |       ^~~~~~~~~~~
> 
> [Description copied from Gustavo A. R. Silva <gustavo@embeddedor.com>
> from his Linux kernel commits.]
> 
> The current codebase makes use of the zero-length array language
> extension to the C90 standard, but the preferred mechanism to declare
> variable-length types such as these ones is a flexible array
> member [1][2], introduced in C99:
> 
>     struct foo {
>             int stuff;
>             struct boo array[];
>     };
> 
> By making use of the mechanism above, we will get a compiler warning
> in case the flexible array does not occur last in the structure, which
> will help us prevent some kind of undefined behavior bugs from being
> inadvertently introduced[3] to the codebase from now on.
> 
> Also, notice that, dynamic memory allocations won't be affected by
> this change:
> 
> "Flexible array members have incomplete type, and so the sizeof operator
> may not be applied. As a quirk of the original implementation of
> zero-length arrays, sizeof evaluates to zero."[1]
> 
> This issue was found with the help of Coccinelle.
> 
> [1] https://gcc.gnu.org/onlinedocs/gcc/Zero-Length.html
> [2] https://github.com/KSPP/linux/issues/21
> [3] commit 76497732932f ("cxgb3/l2t: Fix undefined behaviour")
>     https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=76497732932f15e7323dc805e8ea8dc11bb587cf
> 
> Signed-off-by: Paul Menzel <pmenzel@molgen.mpg.de>
> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
> v2: Cite all references, add Reviewed-by line
> 
>  src/std/tcg.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/src/std/tcg.h b/src/std/tcg.h
> index 1cc1c92..1c9eeb4 100644
> --- a/src/std/tcg.h
> +++ b/src/std/tcg.h
> @@ -524,7 +524,7 @@ struct TCG_EfiSpecIdEventStruct {
>      struct TCG_EfiSpecIdEventAlgorithmSize {
>          u16 algorithmId;
>          u16 digestSize;
> -    } digestSizes[0];
> +    } digestSizes[];
>      /*
>      u8 vendorInfoSize;
>      u8 vendorInfo[0];
> -- 
> 2.25.0
> 



> _______________________________________________
> SeaBIOS mailing list -- seabios@seabios.org
> To unsubscribe send an email to seabios-leave@seabios.org
_______________________________________________
SeaBIOS mailing list -- seabios@seabios.org
To unsubscribe send an email to seabios-leave@seabios.org

[SeaBIOS] Re: [PATCH v2] std/tcg: Replace zero-length array with flexible-array member

Posted by Stefan Berger 5 years, 11 months ago

On 3/6/20 8:33 AM, Kevin O'Connor wrote:
> On Wed, Mar 04, 2020 at 02:51:27PM +0100, Paul Menzel wrote:
>> Date: Tue, 3 Mar 2020 16:24:46 +0100
>>
>> GCC 10 gives the warnings below:
>>
>>      In file included from out/ccode32flat.o.tmp.c:54:
>>      ./src/tcgbios.c: In function 'tpm20_write_EfiSpecIdEventStruct':
>>      ./src/tcgbios.c:290:30: warning: array subscript '(<unknown>) + 4294967295' is outside the bounds of an interior zero-length array 'struct TCG_EfiSpecIdEventAlgorithmSize[0]' [-Wzero-length-bounds]
>>        290 |         event.hdr.digestSizes[count].algorithmId = be16_to_cpu(sel->hashAlg);
>>            |         ~~~~~~~~~~~~~~~~~~~~~^~~~~~~
> Thanks.  It looks fine to me.  Stefan - do you have any comments on this?

Looks good to me, too.


    Stefan

_______________________________________________
SeaBIOS mailing list -- seabios@seabios.org
To unsubscribe send an email to seabios-leave@seabios.org

[SeaBIOS] Re: [PATCH v2] std/tcg: Replace zero-length array with flexible-array member

Posted by Paul Menzel 5 years, 10 months ago

Dear Kevin, dear Gerd,


Am 06.03.20 um 18:01 schrieb Stefan Berger:
> On 3/6/20 8:33 AM, Kevin O'Connor wrote:
>> On Wed, Mar 04, 2020 at 02:51:27PM +0100, Paul Menzel wrote:
>>> Date: Tue, 3 Mar 2020 16:24:46 +0100
>>>
>>> GCC 10 gives the warnings below:
>>>
>>>      In file included from out/ccode32flat.o.tmp.c:54:
>>>      ./src/tcgbios.c: In function 'tpm20_write_EfiSpecIdEventStruct':
>>>      ./src/tcgbios.c:290:30: warning: array subscript '(<unknown>) + 
>>> 4294967295' is outside the bounds of an interior zero-length array 
>>> 'struct TCG_EfiSpecIdEventAlgorithmSize[0]' [-Wzero-length-bounds]
>>>        290 |         event.hdr.digestSizes[count].algorithmId = 
>>> be16_to_cpu(sel->hashAlg);
>>>            |         ~~~~~~~~~~~~~~~~~~~~~^~~~~~~
>> Thanks.  It looks fine to me.  Stefan - do you have any comments on this?
> 
> Looks good to me, too.

Could you please apply this commit to the master branch?


Kind regards,

Paul
_______________________________________________
SeaBIOS mailing list -- seabios@seabios.org
To unsubscribe send an email to seabios-leave@seabios.org

[SeaBIOS] Re: [PATCH v2] std/tcg: Replace zero-length array with flexible-array member

Posted by Kevin O'Connor 5 years, 10 months ago

On Fri, Mar 20, 2020 at 04:41:09PM +0100, Paul Menzel wrote:
> Am 06.03.20 um 18:01 schrieb Stefan Berger:
> > On 3/6/20 8:33 AM, Kevin O'Connor wrote:
> > > On Wed, Mar 04, 2020 at 02:51:27PM +0100, Paul Menzel wrote:
> > > > Date: Tue, 3 Mar 2020 16:24:46 +0100
> > > > 
> > > > GCC 10 gives the warnings below:
> > > > 
> > > >      In file included from out/ccode32flat.o.tmp.c:54:
> > > >      ./src/tcgbios.c: In function 'tpm20_write_EfiSpecIdEventStruct':
> > > >      ./src/tcgbios.c:290:30: warning: array subscript
> > > > '(<unknown>) + 4294967295' is outside the bounds of an interior
> > > > zero-length array 'struct TCG_EfiSpecIdEventAlgorithmSize[0]'
> > > > [-Wzero-length-bounds]
> > > >        290 |         event.hdr.digestSizes[count].algorithmId =
> > > > be16_to_cpu(sel->hashAlg);
> > > >            |         ~~~~~~~~~~~~~~~~~~~~~^~~~~~~
> > > Thanks.  It looks fine to me.  Stefan - do you have any comments on this?
> > 
> > Looks good to me, too.
> 
> Could you please apply this commit to the master branch?

Thanks.  I committed this change.

-Kevin
_______________________________________________
SeaBIOS mailing list -- seabios@seabios.org
To unsubscribe send an email to seabios-leave@seabios.org