* [PATCH] powerpc/rtas: Replace one-element arrays with flexible arrays
@ 2023-01-27 8:50 Andrew Donnellan
2023-01-27 8:59 ` Leonardo Brás
2023-01-27 13:10 ` Nathan Lynch
0 siblings, 2 replies; 6+ messages in thread
From: Andrew Donnellan @ 2023-01-27 8:50 UTC (permalink / raw)
To: linuxppc-dev; +Cc: Nathan Lynch, Leonardo Bras, linux-hardening
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. */
};
--
2.39.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] powerpc/rtas: Replace one-element arrays with flexible arrays
2023-01-27 8:50 [PATCH] powerpc/rtas: Replace one-element arrays with flexible arrays Andrew Donnellan
@ 2023-01-27 8:59 ` Leonardo Brás
2023-01-27 13:10 ` Nathan Lynch
1 sibling, 0 replies; 6+ messages in thread
From: Leonardo Brás @ 2023-01-27 8:59 UTC (permalink / raw)
To: Andrew Donnellan, linuxppc-dev; +Cc: Nathan Lynch, linux-hardening
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>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] powerpc/rtas: Replace one-element arrays with flexible arrays
2023-01-27 8:50 [PATCH] powerpc/rtas: Replace one-element arrays with flexible arrays Andrew Donnellan
2023-01-27 8:59 ` Leonardo Brás
@ 2023-01-27 13:10 ` Nathan Lynch
2023-01-27 19:23 ` Kees Cook
2023-04-20 7:46 ` Andrew Donnellan
1 sibling, 2 replies; 6+ messages in thread
From: Nathan Lynch @ 2023-01-27 13:10 UTC (permalink / raw)
To: Andrew Donnellan, linuxppc-dev; +Cc: Leonardo Bras, linux-hardening
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?
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] powerpc/rtas: Replace one-element arrays with flexible arrays
2023-01-27 13:10 ` Nathan Lynch
@ 2023-01-27 19:23 ` Kees Cook
2023-04-20 7:46 ` Andrew Donnellan
1 sibling, 0 replies; 6+ messages in thread
From: Kees Cook @ 2023-01-27 19:23 UTC (permalink / raw)
To: Nathan Lynch
Cc: Andrew Donnellan, linuxppc-dev, Leonardo Bras, linux-hardening
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/
--
Kees Cook
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] powerpc/rtas: Replace one-element arrays with flexible arrays
@ 2023-01-27 19:23 ` Kees Cook
0 siblings, 0 replies; 6+ messages in thread
From: Kees Cook @ 2023-01-27 19:23 UTC (permalink / raw)
To: Nathan Lynch
Cc: Leonardo Bras, linuxppc-dev, Andrew Donnellan, linux-hardening
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/
--
Kees Cook
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] powerpc/rtas: Replace one-element arrays with flexible arrays
2023-01-27 13:10 ` Nathan Lynch
2023-01-27 19:23 ` Kees Cook
@ 2023-04-20 7:46 ` Andrew Donnellan
1 sibling, 0 replies; 6+ messages in thread
From: Andrew Donnellan @ 2023-04-20 7:46 UTC (permalink / raw)
To: Nathan Lynch, linuxppc-dev; +Cc: Leonardo Bras, linux-hardening
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
--
Andrew Donnellan OzLabs, ADL Canberra
ajd@linux.ibm.com IBM Australia Limited
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2023-04-20 7:47 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-01-27 8:50 [PATCH] powerpc/rtas: Replace one-element arrays with flexible arrays Andrew Donnellan
2023-01-27 8:59 ` Leonardo Brás
2023-01-27 13:10 ` Nathan Lynch
2023-01-27 19:23 ` Kees Cook
2023-01-27 19:23 ` Kees Cook
2023-04-20 7:46 ` Andrew Donnellan
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.