| Message ID | 20200507040912.GA31382@embeddedor (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [RESEND] tpm: eventlog: Replace zero-length array with flexible-array member | expand |
On Wed, May 06, 2020 at 11:09:12PM -0500, Gustavo A. R. Silva wrote: > As mentioned above: "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] So, > the sizeof(flexible-array) can be safely removed to fix the error above. As in "sizeof(event_header->event) always evaluated to 0, so removing it has no effect". > [...] > diff --git a/drivers/char/tpm/eventlog/tpm2.c b/drivers/char/tpm/eventlog/tpm2.c > index e741b1157525..351a2989b3c6 100644 > --- a/drivers/char/tpm/eventlog/tpm2.c > +++ b/drivers/char/tpm/eventlog/tpm2.c > @@ -51,8 +51,7 @@ static void *tpm2_bios_measurements_start(struct seq_file *m, loff_t *pos) > int i; > > event_header = addr; > - size = sizeof(struct tcg_pcr_event) - sizeof(event_header->event) > - + event_header->event_size; > + size = sizeof(*event_header) + event_header->event_size; That said, I think it would be better to stick to the struct_size() idiom for dealing with flexible arrays here: size = struct_size(event_header, event, event_size);
On Thu, May 07, 2020 at 11:02:18AM -0700, Kees Cook wrote: > On Wed, May 06, 2020 at 11:09:12PM -0500, Gustavo A. R. Silva wrote: > > As mentioned above: "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] So, > > the sizeof(flexible-array) can be safely removed to fix the error above. > > As in "sizeof(event_header->event) always evaluated to 0, so removing it > has no effect". > Thanks for this. I wanted to make a more general statement, but I'll update the changelog text. :) > > [...] > > diff --git a/drivers/char/tpm/eventlog/tpm2.c b/drivers/char/tpm/eventlog/tpm2.c > > index e741b1157525..351a2989b3c6 100644 > > --- a/drivers/char/tpm/eventlog/tpm2.c > > +++ b/drivers/char/tpm/eventlog/tpm2.c > > @@ -51,8 +51,7 @@ static void *tpm2_bios_measurements_start(struct seq_file *m, loff_t *pos) > > int i; > > > > event_header = addr; > > - size = sizeof(struct tcg_pcr_event) - sizeof(event_header->event) > > - + event_header->event_size; > > + size = sizeof(*event_header) + event_header->event_size; > > That said, I think it would be better to stick to the struct_size() > idiom for dealing with flexible arrays here: > > size = struct_size(event_header, event, event_size); > Yep, I agree. I'll add this and send v2, shortly. Thanks -- Gustavo
On Wed, May 06, 2020 at 11:09:12PM -0500, Gustavo A. R. Silva wrote: > 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] > > sizeof(flexible-array-member) triggers a warning because flexible array > members have incomplete type[1]. There are some instances of code in > which the sizeof operator is being incorrectly/erroneously applied to > zero-length arrays and the result is zero. Such instances may be hiding > some bugs. So, this work (flexible-array member conversions) will also > help to get completely rid of those sorts of issues. > > Also, the following issue shows up due to the flexible-array member > having incomplete type[4]: > > drivers/char/tpm/eventlog/tpm2.c: In function ‘tpm2_bios_measurements_start’: > drivers/char/tpm/eventlog/tpm2.c:54:46: error: invalid application of ‘sizeof’ to incomplete type ‘u8[]’ {aka ‘unsigned char[]’} > 54 | size = sizeof(struct tcg_pcr_event) - sizeof(event_header->event) > | ^ > drivers/char/tpm/eventlog/tpm2.c: In function ‘tpm2_bios_measurements_next’: > drivers/char/tpm/eventlog/tpm2.c:102:10: error: invalid application of ‘sizeof’ to incomplete type ‘u8[]’ {aka ‘unsigned char[]’} > 102 | sizeof(event_header->event) + event_header->event_size; > | ^ > drivers/char/tpm/eventlog/tpm2.c: In function ‘tpm2_binary_bios_measurements_show’: > drivers/char/tpm/eventlog/tpm2.c:140:10: error: invalid application of ‘sizeof’ to incomplete type ‘u8[]’ {aka ‘unsigned char[]’} > 140 | sizeof(event_header->event) + event_header->event_size; > | ^ > scripts/Makefile.build:266: recipe for target 'drivers/char/tpm/eventlog/tpm2.o' failed > make[3]: *** [drivers/char/tpm/eventlog/tpm2.o] Error 1 > > As mentioned above: "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] So, > the sizeof(flexible-array) can be safely removed to fix the error above. > > Lastly, prefer sizeof(*ptr) over sizeof(struct foo). > > 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") > [4] https://github.com/KSPP/linux/issues/43 > > Signed-off-by: Gustavo A. R. Silva <gustavo@embeddedor.com> Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> /Jarkko
On Thu, May 14, 2020 at 12:39:05AM +0300, Jarkko Sakkinen wrote: > > > > [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") > > [4] https://github.com/KSPP/linux/issues/43 > > > > Signed-off-by: Gustavo A. R. Silva <gustavo@embeddedor.com> > > Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> > Hi Jarkko, Thanks for your RB. There is a v2 of this patch: https://lore.kernel.org/lkml/20200508163826.GA768@embeddedor/ Thanks -- Gustavo
On Wed, 2020-05-13 at 18:14 -0500, Gustavo A. R. Silva wrote: > On Thu, May 14, 2020 at 12:39:05AM +0300, Jarkko Sakkinen wrote: > > > [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") > > > [4] https://github.com/KSPP/linux/issues/43 > > > > > > Signed-off-by: Gustavo A. R. Silva <gustavo@embeddedor.com> > > > > Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> > > > > Hi Jarkko, > > Thanks for your RB. > > There is a v2 of this patch: > > https://lore.kernel.org/lkml/20200508163826.GA768@embeddedor/ > > Thanks > -- > Gustavo Yup, http://git.infradead.org/users/jjs/linux-tpmdd.git/commit/47c18d91f4adb2ca164c8dbee861543b4466167d Does this look correct? /Jarkko
diff --git a/drivers/char/tpm/eventlog/tpm2.c b/drivers/char/tpm/eventlog/tpm2.c index e741b1157525..351a2989b3c6 100644 --- a/drivers/char/tpm/eventlog/tpm2.c +++ b/drivers/char/tpm/eventlog/tpm2.c @@ -51,8 +51,7 @@ static void *tpm2_bios_measurements_start(struct seq_file *m, loff_t *pos) int i; event_header = addr; - size = sizeof(struct tcg_pcr_event) - sizeof(event_header->event) - + event_header->event_size; + size = sizeof(*event_header) + event_header->event_size; if (*pos == 0) { if (addr + size < limit) { @@ -98,8 +97,7 @@ static void *tpm2_bios_measurements_next(struct seq_file *m, void *v, event_header = log->bios_event_log; if (v == SEQ_START_TOKEN) { - event_size = sizeof(struct tcg_pcr_event) - - sizeof(event_header->event) + event_header->event_size; + event_size = sizeof(*event_header) + event_header->event_size; marker = event_header; } else { event = v; @@ -136,9 +134,7 @@ static int tpm2_binary_bios_measurements_show(struct seq_file *m, void *v) size_t size; if (v == SEQ_START_TOKEN) { - size = sizeof(struct tcg_pcr_event) - - sizeof(event_header->event) + event_header->event_size; - + size = sizeof(*event_header) + event_header->event_size; temp_ptr = event_header; if (size > 0) diff --git a/include/linux/tpm_eventlog.h b/include/linux/tpm_eventlog.h index c253461b1c4e..4f8c90c93c29 100644 --- a/include/linux/tpm_eventlog.h +++ b/include/linux/tpm_eventlog.h @@ -97,7 +97,7 @@ struct tcg_pcr_event { u32 event_type; u8 digest[20]; u32 event_size; - u8 event[0]; + u8 event[]; } __packed; struct tcg_event_field {
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] sizeof(flexible-array-member) triggers a warning because flexible array members have incomplete type[1]. There are some instances of code in which the sizeof operator is being incorrectly/erroneously applied to zero-length arrays and the result is zero. Such instances may be hiding some bugs. So, this work (flexible-array member conversions) will also help to get completely rid of those sorts of issues. Also, the following issue shows up due to the flexible-array member having incomplete type[4]: drivers/char/tpm/eventlog/tpm2.c: In function ‘tpm2_bios_measurements_start’: drivers/char/tpm/eventlog/tpm2.c:54:46: error: invalid application of ‘sizeof’ to incomplete type ‘u8[]’ {aka ‘unsigned char[]’} 54 | size = sizeof(struct tcg_pcr_event) - sizeof(event_header->event) | ^ drivers/char/tpm/eventlog/tpm2.c: In function ‘tpm2_bios_measurements_next’: drivers/char/tpm/eventlog/tpm2.c:102:10: error: invalid application of ‘sizeof’ to incomplete type ‘u8[]’ {aka ‘unsigned char[]’} 102 | sizeof(event_header->event) + event_header->event_size; | ^ drivers/char/tpm/eventlog/tpm2.c: In function ‘tpm2_binary_bios_measurements_show’: drivers/char/tpm/eventlog/tpm2.c:140:10: error: invalid application of ‘sizeof’ to incomplete type ‘u8[]’ {aka ‘unsigned char[]’} 140 | sizeof(event_header->event) + event_header->event_size; | ^ scripts/Makefile.build:266: recipe for target 'drivers/char/tpm/eventlog/tpm2.o' failed make[3]: *** [drivers/char/tpm/eventlog/tpm2.o] Error 1 As mentioned above: "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] So, the sizeof(flexible-array) can be safely removed to fix the error above. Lastly, prefer sizeof(*ptr) over sizeof(struct foo). 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") [4] https://github.com/KSPP/linux/issues/43 Signed-off-by: Gustavo A. R. Silva <gustavo@embeddedor.com> --- Hi, I'm resending this because LKML is eating some messages, recently. Sorry for the noise in case you've already received this patch. Thanks drivers/char/tpm/eventlog/tpm2.c | 10 +++------- include/linux/tpm_eventlog.h | 2 +- 2 files changed, 4 insertions(+), 8 deletions(-)