Fix size comparison bug and use flexible array
Subsystems (3)
cover Fix size comparison bug and use flexible array
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(-)
Signature · 1 line
2.27.0
PATCH 1/2 media: staging/intel-ipu3: css: Fix wrong size comparison
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 *'
--- drivers/staging/media/ipu3/ipu3-css-fw.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/staging/media/ipu3/ipu3-css-fw.c +1 -1
@@ -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) +
Signature · 1 line
2.27.0
PATCH 2/2 media: staging/intel-ipu3: css: Replace one-element array and use struct_size() helper
In reply to Gustavo A. R. SilvaThere 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
--- 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(-)
drivers/staging/media/ipu3/ipu3-css-fw.c +2 -3
@@ -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",
drivers/staging/media/ipu3/ipu3-css-fw.h +1 -1
@@ -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 *******************/
Signature · 1 line
2.27.0
Re: [PATCH 2/2][next] media: staging/intel-ipu3: css: Replace one-element array and use struct_size() helper
Quoted 27 lines · 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_headerand use thestruct_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_SOURCEroutines onmemcpy().[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--- 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(-)drivers/staging/media/ipu3/ipu3-css-fw.c +0
-0
drivers/staging/media/ipu3/ipu3-css-fw.c int imgu_css_fw_init(struct imgu_css *css) · line 127
Quoted 1 line
drivers/staging/media/ipu3/ipu3-css-fw.c +0
-0@@ -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
Re: [PATCH 2/2][next] media: staging/intel-ipu3: css: Replace one-element array and use struct_size() helper
Quoted 30 lines · Dan Carpenter
Quoted 27 lines · 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_headerand use thestruct_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_SOURCEroutines onmemcpy().[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--- 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(-)drivers/staging/media/ipu3/ipu3-css-fw.c +0
-0drivers/staging/media/ipu3/ipu3-css-fw.c int imgu_css_fw_init(struct imgu_css *css) · line 127
Quoted 1 line
drivers/staging/media/ipu3/ipu3-css-fw.c +0
-0@@ -127,9 +127,8 @@ int imgu_css_fw_init(struct imgu_css *css) if (css->fw->size < sizeof(struct imgu_fw_header) ||^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Quoted 8 lines
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
Sakari Ailus
maintainerRe: [PATCH 2/2][next] media: staging/intel-ipu3: css: Replace one-element array and use struct_size() helper
Hi Gustavo,
Quoted 40 lines · Gustavo A. R. Silva
Quoted 30 lines · Dan Carpenter
Quoted 27 lines · 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_headerand use thestruct_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_SOURCEroutines onmemcpy().[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--- 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(-)drivers/staging/media/ipu3/ipu3-css-fw.c +0
-0drivers/staging/media/ipu3/ipu3-css-fw.c int imgu_css_fw_init(struct imgu_css *css) · line 127
Quoted 1 line
drivers/staging/media/ipu3/ipu3-css-fw.c +0
-0@@ -127,9 +127,8 @@ int imgu_css_fw_init(struct imgu_css *css) if (css->fw->size < sizeof(struct imgu_fw_header) ||^^^^^^^^^^^^^^^^^^^^^^^^^^^^^Quoted 8 lines
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.
Quoted 5 lines
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.
Signature · 3 lines
Kind regards, Sakari Ailus