| Message ID | 20230127085023.271674-1-ajd@linux.ibm.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | powerpc/rtas: Replace one-element arrays with flexible arrays | expand |
| Context | Check | Description |
|---|---|---|
| snowpatch_ozlabs/github-powerpc_ppctests | success | Successfully ran 8 jobs. |
| snowpatch_ozlabs/github-powerpc_selftests | success | Successfully ran 8 jobs. |
| snowpatch_ozlabs/github-powerpc_sparse | success | Successfully ran 4 jobs. |
| snowpatch_ozlabs/github-powerpc_clang | success | Successfully ran 6 jobs. |
| snowpatch_ozlabs/github-powerpc_kernel_qemu | success | Successfully ran 24 jobs. |
On Fri, 2023-01-27 at 19:50 +1100, Andrew Donnellan wrote: > Using a one-element array as a fake flexible array is deprecated. > > Replace the one-element flexible arrays in rtas-types.h with C99 standard > flexible array members instead. > > This helps us move towards enabling -fstrict-flex-arrays=3 in future. > > Found using scripts/coccinelle/misc/flexible_array.cocci. > > Cc: Nathan Lynch <nathanl@linux.ibm.com> > Cc: Leonardo Bras <leobras.c@gmail.com> > Cc: linux-hardening@vger.kernel.org > Link: https://github.com/KSPP/linux/issues/21 > Link: https://github.com/KSPP/linux/issues/79 > Signed-off-by: Andrew Donnellan <ajd@linux.ibm.com> > --- > arch/powerpc/include/asm/rtas-types.h | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/arch/powerpc/include/asm/rtas-types.h b/arch/powerpc/include/asm/rtas-types.h > index 8df6235d64d1..40ec03a05c0b 100644 > --- a/arch/powerpc/include/asm/rtas-types.h > +++ b/arch/powerpc/include/asm/rtas-types.h > @@ -44,7 +44,7 @@ struct rtas_error_log { > */ > u8 byte3; /* General event or error*/ > __be32 extended_log_length; /* length in bytes */ > - unsigned char buffer[1]; /* Start of extended log */ > + unsigned char buffer[]; /* Start of extended log */ > /* Variable length. */ > }; > > @@ -85,7 +85,7 @@ struct rtas_ext_event_log_v6 { > /* that defines the format for */ > /* the vendor specific log type */ > /* Byte 16-end of log */ > - u8 vendor_log[1]; /* Start of vendor specific log */ > + u8 vendor_log[]; /* Start of vendor specific log */ > /* Variable length. */ > }; > LGTM. FWIW: Reviewed-by: Leonardo Bras <leobras.c@gmail.com>
Andrew Donnellan <ajd@linux.ibm.com> writes: > Using a one-element array as a fake flexible array is deprecated. > > Replace the one-element flexible arrays in rtas-types.h with C99 standard > flexible array members instead. > > This helps us move towards enabling -fstrict-flex-arrays=3 in future. > > Found using scripts/coccinelle/misc/flexible_array.cocci. > > Cc: Nathan Lynch <nathanl@linux.ibm.com> > Cc: Leonardo Bras <leobras.c@gmail.com> > Cc: linux-hardening@vger.kernel.org > Link: https://github.com/KSPP/linux/issues/21 > Link: https://github.com/KSPP/linux/issues/79 > Signed-off-by: Andrew Donnellan <ajd@linux.ibm.com> > --- > arch/powerpc/include/asm/rtas-types.h | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/arch/powerpc/include/asm/rtas-types.h b/arch/powerpc/include/asm/rtas-types.h > index 8df6235d64d1..40ec03a05c0b 100644 > --- a/arch/powerpc/include/asm/rtas-types.h > +++ b/arch/powerpc/include/asm/rtas-types.h > @@ -44,7 +44,7 @@ struct rtas_error_log { > */ > u8 byte3; /* General event or error*/ > __be32 extended_log_length; /* length in bytes */ > - unsigned char buffer[1]; /* Start of extended log */ > + unsigned char buffer[]; /* Start of extended log */ > /* Variable length. */ > }; > > @@ -85,7 +85,7 @@ struct rtas_ext_event_log_v6 { > /* that defines the format for */ > /* the vendor specific log type */ > /* Byte 16-end of log */ > - u8 vendor_log[1]; /* Start of vendor specific log */ > + u8 vendor_log[]; /* Start of vendor specific log */ > /* Variable length. */ > }; I see at least one place that consults the size of one of these structs, in get_pseries_errorlog(): /* Check that we understand the format */ if (ext_log_length < sizeof(struct rtas_ext_event_log_v6) || ... Don't all such sites need to be audited/adjusted for changes like this?
On Fri, Jan 27, 2023 at 07:10:28AM -0600, Nathan Lynch wrote: > Andrew Donnellan <ajd@linux.ibm.com> writes: > > Using a one-element array as a fake flexible array is deprecated. > > > > Replace the one-element flexible arrays in rtas-types.h with C99 standard > > flexible array members instead. > > > > This helps us move towards enabling -fstrict-flex-arrays=3 in future. > > > > Found using scripts/coccinelle/misc/flexible_array.cocci. > > > > Cc: Nathan Lynch <nathanl@linux.ibm.com> > > Cc: Leonardo Bras <leobras.c@gmail.com> > > Cc: linux-hardening@vger.kernel.org > > Link: https://github.com/KSPP/linux/issues/21 > > Link: https://github.com/KSPP/linux/issues/79 > > Signed-off-by: Andrew Donnellan <ajd@linux.ibm.com> > > --- > > arch/powerpc/include/asm/rtas-types.h | 4 ++-- > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/arch/powerpc/include/asm/rtas-types.h b/arch/powerpc/include/asm/rtas-types.h > > index 8df6235d64d1..40ec03a05c0b 100644 > > --- a/arch/powerpc/include/asm/rtas-types.h > > +++ b/arch/powerpc/include/asm/rtas-types.h > > @@ -44,7 +44,7 @@ struct rtas_error_log { > > */ > > u8 byte3; /* General event or error*/ > > __be32 extended_log_length; /* length in bytes */ > > - unsigned char buffer[1]; /* Start of extended log */ > > + unsigned char buffer[]; /* Start of extended log */ > > /* Variable length. */ > > }; > > > > @@ -85,7 +85,7 @@ struct rtas_ext_event_log_v6 { > > /* that defines the format for */ > > /* the vendor specific log type */ > > /* Byte 16-end of log */ > > - u8 vendor_log[1]; /* Start of vendor specific log */ > > + u8 vendor_log[]; /* Start of vendor specific log */ > > /* Variable length. */ > > }; > > I see at least one place that consults the size of one of these structs, > in get_pseries_errorlog(): > > /* Check that we understand the format */ > if (ext_log_length < sizeof(struct rtas_ext_event_log_v6) || ... > > Don't all such sites need to be audited/adjusted for changes like this? Yeah, I'd expect a binary comparison[1] before/after to catch things like this. E.g. the following C files mention those structs: arch/powerpc/platforms/pseries/io_event_irq.c arch/powerpc/platforms/pseries/ras.c arch/powerpc/kernel/rtasd.c arch/powerpc/kernel/rtas.c -Kees [1] https://outflux.net/blog/archives/2022/06/24/finding-binary-differences/
On Fri, 2023-01-27 at 07:10 -0600, Nathan Lynch wrote: > > > > I see at least one place that consults the size of one of these > > > > structs, > > > > in get_pseries_errorlog(): > > > > > > > > /* Check that we understand the format */ > > > > if (ext_log_length < sizeof(struct > > > > rtas_ext_event_log_v6) > > > > || > > > > ... > > > > > > > > Don't all such sites need to be audited/adjusted for changes > > > > like > > > > this? I did actually see that site, and concluded that for the purposes of that particular check, removing a single extra byte is irrelevant (maybe it makes the check more strictly correct, what if the vendor_log is actually of length 0?) Doing a binary diff, as Kees suggests, over the object files in arch/powerpc: - there's no difference at all caused by changing rtas_ext_event_log_v6.vendor_log, which kind of surprises me given the above. - changing rtas_error_log.buffer does seem to change some code generation in arch/powerpc/platforms/pseries/ras.o, I can't quite see why. Andrew
diff --git a/arch/powerpc/include/asm/rtas-types.h b/arch/powerpc/include/asm/rtas-types.h index 8df6235d64d1..40ec03a05c0b 100644 --- a/arch/powerpc/include/asm/rtas-types.h +++ b/arch/powerpc/include/asm/rtas-types.h @@ -44,7 +44,7 @@ struct rtas_error_log { */ u8 byte3; /* General event or error*/ __be32 extended_log_length; /* length in bytes */ - unsigned char buffer[1]; /* Start of extended log */ + unsigned char buffer[]; /* Start of extended log */ /* Variable length. */ }; @@ -85,7 +85,7 @@ struct rtas_ext_event_log_v6 { /* that defines the format for */ /* the vendor specific log type */ /* Byte 16-end of log */ - u8 vendor_log[1]; /* Start of vendor specific log */ + u8 vendor_log[]; /* Start of vendor specific log */ /* Variable length. */ };
Using a one-element array as a fake flexible array is deprecated. Replace the one-element flexible arrays in rtas-types.h with C99 standard flexible array members instead. This helps us move towards enabling -fstrict-flex-arrays=3 in future. Found using scripts/coccinelle/misc/flexible_array.cocci. Cc: Nathan Lynch <nathanl@linux.ibm.com> Cc: Leonardo Bras <leobras.c@gmail.com> Cc: linux-hardening@vger.kernel.org Link: https://github.com/KSPP/linux/issues/21 Link: https://github.com/KSPP/linux/issues/79 Signed-off-by: Andrew Donnellan <ajd@linux.ibm.com> --- arch/powerpc/include/asm/rtas-types.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)