linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/2][next] Fix size comparison bug and use flexible array
@ 2021-07-29 23:19 Gustavo A. R. Silva
  2021-07-29 23:20 ` [PATCH 1/2][next] media: staging/intel-ipu3: css: Fix wrong size comparison Gustavo A. R. Silva
  2021-07-29 23:22 ` [PATCH 2/2][next] media: staging/intel-ipu3: css: Replace one-element array and use struct_size() helper Gustavo A. R. Silva
  0 siblings, 2 replies; 6+ messages in thread
From: Gustavo A. R. Silva @ 2021-07-29 23:19 UTC (permalink / raw)
  To: linux-kernel
  Cc: Yong Zhi, Sakari Ailus, Bingbu Cao, Tianshu Qiu,
	Mauro Carvalho Chehab, Greg Kroah-Hartman, linux-media,
	linux-staging, linux-hardening, Gustavo A. R. Silva

Hi all,

This small series aims to fix a size comparison bug and replace a
one-element array with a flexible-array member.

Thanks

Gustavo A. R. Silva (2):
  media: staging/intel-ipu3: css: Fix wrong size comparison
  media: staging/intel-ipu3: css: Replace one-element array and use
    struct_size() helper

 drivers/staging/media/ipu3/ipu3-css-fw.c | 7 +++----
 drivers/staging/media/ipu3/ipu3-css-fw.h | 2 +-
 2 files changed, 4 insertions(+), 5 deletions(-)

-- 
2.27.0


^ permalink raw reply	[flat|nested] 6+ messages in thread

* [PATCH 1/2][next] media: staging/intel-ipu3: css: Fix wrong size comparison
  2021-07-29 23:19 [PATCH 0/2][next] Fix size comparison bug and use flexible array Gustavo A. R. Silva
@ 2021-07-29 23:20 ` Gustavo A. R. Silva
  2021-07-29 23:22 ` [PATCH 2/2][next] media: staging/intel-ipu3: css: Replace one-element array and use struct_size() helper Gustavo A. R. Silva
  1 sibling, 0 replies; 6+ messages in thread
From: Gustavo A. R. Silva @ 2021-07-29 23:20 UTC (permalink / raw)
  To: linux-kernel
  Cc: Yong Zhi, Sakari Ailus, Bingbu Cao, Tianshu Qiu,
	Mauro Carvalho Chehab, Greg Kroah-Hartman, linux-media,
	linux-staging, linux-hardening, Gustavo A. R. Silva

There is a wrong comparison of the total size of the loaded firmware
css->fw->size with the size of a pointer to struct imgu_fw_header.

Fix this by using the right operand 'struct imgu_fw_header' for
sizeof, instead of 'struct imgu_fw_header *'

Fixes: 09d290f0ba21 ("media: staging/intel-ipu3: css: Add support for firmware management")
Cc: stable@vger.kernel.org
Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org>
---
 drivers/staging/media/ipu3/ipu3-css-fw.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/media/ipu3/ipu3-css-fw.c b/drivers/staging/media/ipu3/ipu3-css-fw.c
index 45aff76198e2..ab021afff954 100644
--- a/drivers/staging/media/ipu3/ipu3-css-fw.c
+++ b/drivers/staging/media/ipu3/ipu3-css-fw.c
@@ -124,7 +124,7 @@ int imgu_css_fw_init(struct imgu_css *css)
 	/* Check and display fw header info */
 
 	css->fwp = (struct imgu_fw_header *)css->fw->data;
-	if (css->fw->size < sizeof(struct imgu_fw_header *) ||
+	if (css->fw->size < sizeof(struct imgu_fw_header) ||
 	    css->fwp->file_header.h_size != sizeof(struct imgu_fw_bi_file_h))
 		goto bad_fw;
 	if (sizeof(struct imgu_fw_bi_file_h) +
-- 
2.27.0


^ permalink raw reply	[flat|nested] 6+ messages in thread

* [PATCH 2/2][next] media: staging/intel-ipu3: css: Replace one-element array and use struct_size() helper
  2021-07-29 23:19 [PATCH 0/2][next] Fix size comparison bug and use flexible array Gustavo A. R. Silva
  2021-07-29 23:20 ` [PATCH 1/2][next] media: staging/intel-ipu3: css: Fix wrong size comparison Gustavo A. R. Silva
@ 2021-07-29 23:22 ` Gustavo A. R. Silva
  2021-07-30  7:49   ` Dan Carpenter
  1 sibling, 1 reply; 6+ messages in thread
From: Gustavo A. R. Silva @ 2021-07-29 23:22 UTC (permalink / raw)
  To: linux-kernel
  Cc: Yong Zhi, Sakari Ailus, Bingbu Cao, Tianshu Qiu,
	Mauro Carvalho Chehab, Greg Kroah-Hartman, linux-media,
	linux-staging, linux-hardening, Gustavo A. R. Silva

There is a regular need in the kernel to provide a way to declare having
a dynamically sized set of trailing elements in a structure. Kernel code
should always use “flexible array members”[1] for these cases. The older
style of one-element or zero-length arrays should no longer be used[2].

Replace a one-element array with a flexible-array member in struct
imgu_fw_header and use the struct_size() helper.

This also helps with the ongoing efforts to globally enable
-Warray-bounds and get us closer to being able to tighten the
FORTIFY_SOURCE routines on memcpy().

[1] https://en.wikipedia.org/wiki/Flexible_array_member
[2] https://www.kernel.org/doc/html/v5.10/process/deprecated.html#zero-length-and-one-element-arrays

Link: https://github.com/KSPP/linux/issues/79
Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org>
---
 drivers/staging/media/ipu3/ipu3-css-fw.c | 5 ++---
 drivers/staging/media/ipu3/ipu3-css-fw.h | 2 +-
 2 files changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/staging/media/ipu3/ipu3-css-fw.c b/drivers/staging/media/ipu3/ipu3-css-fw.c
index ab021afff954..3b7df1128840 100644
--- a/drivers/staging/media/ipu3/ipu3-css-fw.c
+++ b/drivers/staging/media/ipu3/ipu3-css-fw.c
@@ -127,9 +127,8 @@ int imgu_css_fw_init(struct imgu_css *css)
 	if (css->fw->size < sizeof(struct imgu_fw_header) ||
 	    css->fwp->file_header.h_size != sizeof(struct imgu_fw_bi_file_h))
 		goto bad_fw;
-	if (sizeof(struct imgu_fw_bi_file_h) +
-	    css->fwp->file_header.binary_nr * sizeof(struct imgu_fw_info) >
-	    css->fw->size)
+	if (struct_size(css->fwp, binary_header,
+			css->fwp->file_header.binary_nr) > css->fw->size)
 		goto bad_fw;
 
 	dev_info(dev, "loaded firmware version %.64s, %u binaries, %zu bytes\n",
diff --git a/drivers/staging/media/ipu3/ipu3-css-fw.h b/drivers/staging/media/ipu3/ipu3-css-fw.h
index 3c078f15a295..c0bc57fd678a 100644
--- a/drivers/staging/media/ipu3/ipu3-css-fw.h
+++ b/drivers/staging/media/ipu3/ipu3-css-fw.h
@@ -171,7 +171,7 @@ struct imgu_fw_bi_file_h {
 
 struct imgu_fw_header {
 	struct imgu_fw_bi_file_h file_header;
-	struct imgu_fw_info binary_header[1];	/* binary_nr items */
+	struct imgu_fw_info binary_header[];	/* binary_nr items */
 };
 
 /******************* Firmware functions *******************/
-- 
2.27.0


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH 2/2][next] media: staging/intel-ipu3: css: Replace one-element array and use struct_size() helper
  2021-07-29 23:22 ` [PATCH 2/2][next] media: staging/intel-ipu3: css: Replace one-element array and use struct_size() helper Gustavo A. R. Silva
@ 2021-07-30  7:49   ` Dan Carpenter
  2021-07-30  8:40     ` Gustavo A. R. Silva
  0 siblings, 1 reply; 6+ messages in thread
From: Dan Carpenter @ 2021-07-30  7:49 UTC (permalink / raw)
  To: Gustavo A. R. Silva
  Cc: linux-kernel, Yong Zhi, Sakari Ailus, Bingbu Cao, Tianshu Qiu,
	Mauro Carvalho Chehab, Greg Kroah-Hartman, linux-media,
	linux-staging, linux-hardening

On Thu, Jul 29, 2021 at 06:22:40PM -0500, Gustavo A. R. Silva wrote:
> There is a regular need in the kernel to provide a way to declare having
> a dynamically sized set of trailing elements in a structure. Kernel code
> should always use “flexible array members”[1] for these cases. The older
> style of one-element or zero-length arrays should no longer be used[2].
> 
> Replace a one-element array with a flexible-array member in struct
> imgu_fw_header and use the struct_size() helper.
> 
> This also helps with the ongoing efforts to globally enable
> -Warray-bounds and get us closer to being able to tighten the
> FORTIFY_SOURCE routines on memcpy().
> 
> [1] https://en.wikipedia.org/wiki/Flexible_array_member
> [2] https://www.kernel.org/doc/html/v5.10/process/deprecated.html#zero-length-and-one-element-arrays
> 
> Link: https://github.com/KSPP/linux/issues/79
> Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org>
> ---
>  drivers/staging/media/ipu3/ipu3-css-fw.c | 5 ++---
>  drivers/staging/media/ipu3/ipu3-css-fw.h | 2 +-
>  2 files changed, 3 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/staging/media/ipu3/ipu3-css-fw.c b/drivers/staging/media/ipu3/ipu3-css-fw.c
> index ab021afff954..3b7df1128840 100644
> --- a/drivers/staging/media/ipu3/ipu3-css-fw.c
> +++ b/drivers/staging/media/ipu3/ipu3-css-fw.c
> @@ -127,9 +127,8 @@ int imgu_css_fw_init(struct imgu_css *css)
>  	if (css->fw->size < sizeof(struct imgu_fw_header) ||
                            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Originally this was sizeof() the pointer which is clearly wrong.  Then
patch 1 changed it to force that binary_header[] had at least one
element, but now it's changed again to say that binary_header[] can have
zero elements.  So either patch 1 or patch 2 is wrong.

I feel like the probably the correct fix is to just fold these two
patches together and say that binary_header[] with zero elements is
allowed.  But I don't know this code well.

regards,
dan carpenter



^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH 2/2][next] media: staging/intel-ipu3: css: Replace one-element array and use struct_size() helper
  2021-07-30  7:49   ` Dan Carpenter
@ 2021-07-30  8:40     ` Gustavo A. R. Silva
  2021-08-02  6:00       ` Sakari Ailus
  0 siblings, 1 reply; 6+ messages in thread
From: Gustavo A. R. Silva @ 2021-07-30  8:40 UTC (permalink / raw)
  To: Dan Carpenter
  Cc: linux-kernel, Yong Zhi, Sakari Ailus, Bingbu Cao, Tianshu Qiu,
	Mauro Carvalho Chehab, Greg Kroah-Hartman, linux-media,
	linux-staging, linux-hardening

On Fri, Jul 30, 2021 at 10:49:50AM +0300, Dan Carpenter wrote:
> On Thu, Jul 29, 2021 at 06:22:40PM -0500, Gustavo A. R. Silva wrote:
> > There is a regular need in the kernel to provide a way to declare having
> > a dynamically sized set of trailing elements in a structure. Kernel code
> > should always use “flexible array members”[1] for these cases. The older
> > style of one-element or zero-length arrays should no longer be used[2].
> > 
> > Replace a one-element array with a flexible-array member in struct
> > imgu_fw_header and use the struct_size() helper.
> > 
> > This also helps with the ongoing efforts to globally enable
> > -Warray-bounds and get us closer to being able to tighten the
> > FORTIFY_SOURCE routines on memcpy().
> > 
> > [1] https://en.wikipedia.org/wiki/Flexible_array_member
> > [2] https://www.kernel.org/doc/html/v5.10/process/deprecated.html#zero-length-and-one-element-arrays
> > 
> > Link: https://github.com/KSPP/linux/issues/79
> > Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org>
> > ---
> >  drivers/staging/media/ipu3/ipu3-css-fw.c | 5 ++---
> >  drivers/staging/media/ipu3/ipu3-css-fw.h | 2 +-
> >  2 files changed, 3 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/staging/media/ipu3/ipu3-css-fw.c b/drivers/staging/media/ipu3/ipu3-css-fw.c
> > index ab021afff954..3b7df1128840 100644
> > --- a/drivers/staging/media/ipu3/ipu3-css-fw.c
> > +++ b/drivers/staging/media/ipu3/ipu3-css-fw.c
> > @@ -127,9 +127,8 @@ int imgu_css_fw_init(struct imgu_css *css)
> >  	if (css->fw->size < sizeof(struct imgu_fw_header) ||
>                             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> Originally this was sizeof() the pointer which is clearly wrong.  Then
> patch 1 changed it to force that binary_header[] had at least one
> element, but now it's changed again to say that binary_header[] can have
> zero elements.  So either patch 1 or patch 2 is wrong.
> 
> I feel like the probably the correct fix is to just fold these two
> patches together and say that binary_header[] with zero elements is
> allowed.  But I don't know this code well.

Oops... I forgot to tag this one for stable, too. But it would probably
be better to fold both of these into a single patch as you suggest.

To me these two pieces of code suggest that binary_header[] should not
be declared as a one-element array, but a flexible one instead:

130                 if (sizeof(struct imgu_fw_bi_file_h) +
131                     css->fwp->file_header.binary_nr * sizeof(struct imgu_fw_info) >
132                     css->fw->size)
133                         goto bad_fw;
...
147                 for (i = 0; i < binary_nr; i++) {
148                         struct imgu_fw_info *bi = &css->fwp->binary_header[i];
...

Thanks
--
Gustavo

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH 2/2][next] media: staging/intel-ipu3: css: Replace one-element array and use struct_size() helper
  2021-07-30  8:40     ` Gustavo A. R. Silva
@ 2021-08-02  6:00       ` Sakari Ailus
  0 siblings, 0 replies; 6+ messages in thread
From: Sakari Ailus @ 2021-08-02  6:00 UTC (permalink / raw)
  To: Gustavo A. R. Silva
  Cc: Dan Carpenter, linux-kernel, Yong Zhi, Bingbu Cao, Tianshu Qiu,
	Mauro Carvalho Chehab, Greg Kroah-Hartman, linux-media,
	linux-staging, linux-hardening

Hi Gustavo,

On Fri, Jul 30, 2021 at 03:40:09AM -0500, Gustavo A. R. Silva wrote:
> On Fri, Jul 30, 2021 at 10:49:50AM +0300, Dan Carpenter wrote:
> > On Thu, Jul 29, 2021 at 06:22:40PM -0500, Gustavo A. R. Silva wrote:
> > > There is a regular need in the kernel to provide a way to declare having
> > > a dynamically sized set of trailing elements in a structure. Kernel code
> > > should always use “flexible array members”[1] for these cases. The older
> > > style of one-element or zero-length arrays should no longer be used[2].
> > > 
> > > Replace a one-element array with a flexible-array member in struct
> > > imgu_fw_header and use the struct_size() helper.
> > > 
> > > This also helps with the ongoing efforts to globally enable
> > > -Warray-bounds and get us closer to being able to tighten the
> > > FORTIFY_SOURCE routines on memcpy().
> > > 
> > > [1] https://en.wikipedia.org/wiki/Flexible_array_member
> > > [2] https://www.kernel.org/doc/html/v5.10/process/deprecated.html#zero-length-and-one-element-arrays
> > > 
> > > Link: https://github.com/KSPP/linux/issues/79
> > > Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org>
> > > ---
> > >  drivers/staging/media/ipu3/ipu3-css-fw.c | 5 ++---
> > >  drivers/staging/media/ipu3/ipu3-css-fw.h | 2 +-
> > >  2 files changed, 3 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/drivers/staging/media/ipu3/ipu3-css-fw.c b/drivers/staging/media/ipu3/ipu3-css-fw.c
> > > index ab021afff954..3b7df1128840 100644
> > > --- a/drivers/staging/media/ipu3/ipu3-css-fw.c
> > > +++ b/drivers/staging/media/ipu3/ipu3-css-fw.c
> > > @@ -127,9 +127,8 @@ int imgu_css_fw_init(struct imgu_css *css)
> > >  	if (css->fw->size < sizeof(struct imgu_fw_header) ||
> >                             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> > Originally this was sizeof() the pointer which is clearly wrong.  Then
> > patch 1 changed it to force that binary_header[] had at least one
> > element, but now it's changed again to say that binary_header[] can have
> > zero elements.  So either patch 1 or patch 2 is wrong.
> > 
> > I feel like the probably the correct fix is to just fold these two
> > patches together and say that binary_header[] with zero elements is
> > allowed.  But I don't know this code well.
> 
> Oops... I forgot to tag this one for stable, too. But it would probably
> be better to fold both of these into a single patch as you suggest.
> 
> To me these two pieces of code suggest that binary_header[] should not
> be declared as a one-element array, but a flexible one instead:

I guess if you look at the data structure alone, you're right.

But you'll need at least one binary for the firmware to be useful. So if
you'd change the struct as in this patch, an additional check for the
number of binaries will be needed.

I think I'd keep the patches separate, the first one is a bugfix but the
second one is just about making the code a little nicer.

-- 
Kind regards,

Sakari Ailus

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2021-08-02  6:00 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-07-29 23:19 [PATCH 0/2][next] Fix size comparison bug and use flexible array Gustavo A. R. Silva
2021-07-29 23:20 ` [PATCH 1/2][next] media: staging/intel-ipu3: css: Fix wrong size comparison Gustavo A. R. Silva
2021-07-29 23:22 ` [PATCH 2/2][next] media: staging/intel-ipu3: css: Replace one-element array and use struct_size() helper Gustavo A. R. Silva
2021-07-30  7:49   ` Dan Carpenter
2021-07-30  8:40     ` Gustavo A. R. Silva
2021-08-02  6:00       ` Sakari Ailus

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).