Fix size comparison bug and use flexible array

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 *' and turn binary_header into a flexible-array member. Also, adjust the relational operator to be '<=' instead of '<', as it seems that the intention of the comparison is to determine if the loaded firmware contains any 'struct imgu_fw_info' items in the binary_header[] array than merely the file_header (struct imgu_fw_bi_file_h).

The replacement of the one-element array with a flexible-array member also help with the ongoing efforts to globally enable -Warray-bounds and get us closer to being able to tighten the FORTIFY_SOURCE routines on memcpy().

Link https://github.com/KSPP/linux/issues/79

Link https://github.com/KSPP/linux/issues/109

Fixes 09d290f0ba21 ("media: staging/intel-ipu3: css: Add support for firmware management")

Cc [email protected]

Signed-off-by Gustavo A. R. Silva <[email protected]>

---

It'd be just great if someone that knows this code better can confirm these changes are correct. In particular the adjustment of the relational operator. Thanks!

Changes in v2:
- Use flexible array and adjust relational operator, accordingly.

- Update changelog text.

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

drivers/staging/media/ipu3/ipu3-css-fw.c +1 -1

index 45aff76198e2..630cb5186b48 100644
@@ -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) +

drivers/staging/media/ipu3/ipu3-css-fw.h +1 -1

index 3c078f15a295..c0bc57fd678a 100644
@@ -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 *******************/

Hi Gustavo,

I missed you already had sent v2...

Quoted 27 lines · 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 *' and turn binary_header into a flexible-array member. Also, adjust the relational operator to be '<=' instead of '<', as it seems that the intention of the comparison is to determine if the loaded firmware contains any 'struct imgu_fw_info' items in the binary_header[] array than merely the file_header (struct imgu_fw_bi_file_h).

The replacement of the one-element array with a flexible-array member also help with the ongoing efforts to globally enable -Warray-bounds and get us closer to being able to tighten the FORTIFY_SOURCE routines on memcpy().

Link https://github.com/KSPP/linux/issues/79

Link https://github.com/KSPP/linux/issues/109

Fixes 09d290f0ba21 ("media: staging/intel-ipu3: css: Add support for firmware management")

Cc [email protected]

Signed-off-by Gustavo A. R. Silva <[email protected]>

---

It'd be just great if someone that knows this code better can confirm these changes are correct. In particular the adjustment of the relational operator. Thanks!

Quoted 2 lines

Changes in v2:
- Use flexible array and adjust relational operator, accordingly.

The operator was just correct. The check is just there to see the firmware is at least as large as the struct as which it is being accessed.

Quoted 33 lines

- Update changelog text.

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

drivers/staging/media/ipu3/ipu3-css-fw.c +1 -1

index 45aff76198e2..630cb5186b48 100644
@@ -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) +

drivers/staging/media/ipu3/ipu3-css-fw.h +1 -1

index 3c078f15a295..c0bc57fd678a 100644
@@ -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 *******************/

Hi Sakari,

Quoted 32 lines · Sakari Ailus

Hi Gustavo,

I missed you already had sent v2...

Quoted 27 lines · 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 *' and turn binary_header into a flexible-array member. Also, adjust the relational operator to be '<=' instead of '<', as it seems that the intention of the comparison is to determine if the loaded firmware contains any 'struct imgu_fw_info' items in the binary_header[] array than merely the file_header (struct imgu_fw_bi_file_h).

The replacement of the one-element array with a flexible-array member also help with the ongoing efforts to globally enable -Warray-bounds and get us closer to being able to tighten the FORTIFY_SOURCE routines on memcpy().

Link https://github.com/KSPP/linux/issues/79

Link https://github.com/KSPP/linux/issues/109

Fixes 09d290f0ba21 ("media: staging/intel-ipu3: css: Add support for firmware management")

Cc [email protected]

Signed-off-by Gustavo A. R. Silva <[email protected]>

---

It'd be just great if someone that knows this code better can confirm these changes are correct. In particular the adjustment of the relational operator. Thanks!

Quoted 5 lines

Quoted 2 lines

Changes in v2:
- Use flexible array and adjust relational operator, accordingly.

The operator was just correct. The check is just there to see the firmware is at least as large as the struct as which it is being accessed.

I'm a bit confused, so based on your reply to v1 of this series, this patch is now correct, right?

The operator in v1 _was_ correct as long as the one-element array wasn't transformed into a flexible array, right?

Notice that generally speaking flexible-array members don't occupy space in the containing structure:

$ pahole -C imgu_fw_header drivers/staging/media/ipu3/ipu3-css-fw.o
struct imgu_fw_header {
	struct imgu_fw_bi_file_h   file_header;          /*     0    72 */
	/* --- cacheline 1 boundary (64 bytes) was 8 bytes ago --- */
	struct imgu_fw_info        binary_header[] __attribute__((__aligned__(8))); /*    72     0 */

	/* size: 72, cachelines: 2, members: 2 */
	/* forced alignments: 1 */
	/* last cacheline: 8 bytes */
} __attribute__((__aligned__(8)));

$ pahole -C imgu_fw_header drivers/staging/media/ipu3/ipu3-css-fw.o
struct imgu_fw_header {
	struct imgu_fw_bi_file_h   file_header;          /*     0    72 */
	/* --- cacheline 1 boundary (64 bytes) was 8 bytes ago --- */
	struct imgu_fw_info        binary_header[1] __attribute__((__aligned__(8))); /*    72  1200 */

	/* size: 1272, cachelines: 20, members: 2 */
	/* forced alignments: 1 */
	/* last cacheline: 56 bytes */
} __attribute__((__aligned__(8)));

So, now that the flexible array transformation is included in the same patch as the bugfix, the operator is changed from '<' to '<='

Hi Gustavo,

Apologies for the delay.

Quoted 72 lines · Gustavo A. R. Silva

Hi Sakari,

Quoted 32 lines · Sakari Ailus

Hi Gustavo,

I missed you already had sent v2...

Quoted 27 lines · 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 *' and turn binary_header into a flexible-array member. Also, adjust the relational operator to be '<=' instead of '<', as it seems that the intention of the comparison is to determine if the loaded firmware contains any 'struct imgu_fw_info' items in the binary_header[] array than merely the file_header (struct imgu_fw_bi_file_h).

The replacement of the one-element array with a flexible-array member also help with the ongoing efforts to globally enable -Warray-bounds and get us closer to being able to tighten the FORTIFY_SOURCE routines on memcpy().

Link https://github.com/KSPP/linux/issues/79

Link https://github.com/KSPP/linux/issues/109

Fixes 09d290f0ba21 ("media: staging/intel-ipu3: css: Add support for firmware management")

Cc [email protected]

Signed-off-by Gustavo A. R. Silva <[email protected]>

---

It'd be just great if someone that knows this code better can confirm these changes are correct. In particular the adjustment of the relational operator. Thanks!

Quoted 5 lines

Quoted 2 lines

Changes in v2:
- Use flexible array and adjust relational operator, accordingly.

The operator was just correct. The check is just there to see the firmware is at least as large as the struct as which it is being accessed.

I'm a bit confused, so based on your reply to v1 of this series, this patch is now correct, right?

The operator in v1 _was_ correct as long as the one-element array wasn't transformed into a flexible array, right?

Notice that generally speaking flexible-array members don't occupy space in the containing structure:

$ pahole -C imgu_fw_header drivers/staging/media/ipu3/ipu3-css-fw.o
struct imgu_fw_header {
	struct imgu_fw_bi_file_h   file_header;          /*     0    72 */
	/* --- cacheline 1 boundary (64 bytes) was 8 bytes ago --- */
	struct imgu_fw_info        binary_header[] __attribute__((__aligned__(8))); /*    72     0 */

	/* size: 72, cachelines: 2, members: 2 */
	/* forced alignments: 1 */
	/* last cacheline: 8 bytes */
} __attribute__((__aligned__(8)));

$ pahole -C imgu_fw_header drivers/staging/media/ipu3/ipu3-css-fw.o
struct imgu_fw_header {
	struct imgu_fw_bi_file_h   file_header;          /*     0    72 */
	/* --- cacheline 1 boundary (64 bytes) was 8 bytes ago --- */
	struct imgu_fw_info        binary_header[1] __attribute__((__aligned__(8))); /*    72  1200 */

	/* size: 1272, cachelines: 20, members: 2 */
	/* forced alignments: 1 */
	/* last cacheline: 56 bytes */
} __attribute__((__aligned__(8)));

Quoted 2 lines

So, now that the flexible array transformation is included in the same patch as the bugfix, the operator is changed from '<' to '<='

'<' is correct since you only need as much data as the struct you're about to access is large, not a byte more than that. As Dan noted.

I think you could add a check for binary_nr is at least one.

Hi Sakari,

Please, see my comments below...

Quoted 77 lines · Sakari Ailus

Hi Gustavo,

Apologies for the delay.

Quoted 72 lines · Gustavo A. R. Silva

Hi Sakari,

Quoted 32 lines · Sakari Ailus

Hi Gustavo,

I missed you already had sent v2...

Quoted 27 lines · 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 *' and turn binary_header into a flexible-array member. Also, adjust the relational operator to be '<=' instead of '<', as it seems that the intention of the comparison is to determine if the loaded firmware contains any 'struct imgu_fw_info' items in the binary_header[] array than merely the file_header (struct imgu_fw_bi_file_h).

The replacement of the one-element array with a flexible-array member also help with the ongoing efforts to globally enable -Warray-bounds and get us closer to being able to tighten the FORTIFY_SOURCE routines on memcpy().

Link https://github.com/KSPP/linux/issues/79

Link https://github.com/KSPP/linux/issues/109

Fixes 09d290f0ba21 ("media: staging/intel-ipu3: css: Add support for firmware management")

Cc [email protected]

Signed-off-by Gustavo A. R. Silva <[email protected]>

---

It'd be just great if someone that knows this code better can confirm these changes are correct. In particular the adjustment of the relational operator. Thanks!

Quoted 5 lines

Quoted 2 lines

Changes in v2:
- Use flexible array and adjust relational operator, accordingly.

The operator was just correct. The check is just there to see the firmware is at least as large as the struct as which it is being accessed.

I'm a bit confused, so based on your reply to v1 of this series, this patch is now correct, right?

The operator in v1 _was_ correct as long as the one-element array wasn't transformed into a flexible array, right?

Notice that generally speaking flexible-array members don't occupy space in the containing structure:

$ pahole -C imgu_fw_header drivers/staging/media/ipu3/ipu3-css-fw.o
struct imgu_fw_header {
	struct imgu_fw_bi_file_h   file_header;          /*     0    72 */
	/* --- cacheline 1 boundary (64 bytes) was 8 bytes ago --- */
	struct imgu_fw_info        binary_header[] __attribute__((__aligned__(8))); /*    72     0 */

	/* size: 72, cachelines: 2, members: 2 */
	/* forced alignments: 1 */
	/* last cacheline: 8 bytes */
} __attribute__((__aligned__(8)));

$ pahole -C imgu_fw_header drivers/staging/media/ipu3/ipu3-css-fw.o
struct imgu_fw_header {
	struct imgu_fw_bi_file_h   file_header;          /*     0    72 */
	/* --- cacheline 1 boundary (64 bytes) was 8 bytes ago --- */
	struct imgu_fw_info        binary_header[1] __attribute__((__aligned__(8))); /*    72  1200 */

	/* size: 1272, cachelines: 20, members: 2 */
	/* forced alignments: 1 */
	/* last cacheline: 56 bytes */
} __attribute__((__aligned__(8)));

Quoted 7 lines

Quoted 2 lines

So, now that the flexible array transformation is included in the same patch as the bugfix, the operator is changed from '<' to '<='

'<' is correct since you only need as much data as the struct you're about to access is large, not a byte more than that. As Dan noted.

I think you could add a check for binary_nr is at least one.

If we need to check that binary_nr is at least one, then this would be the right change:

        css->fwp = (struct imgu_fw_header *)css->fw->data;

Quoted 93 lines · Gustavo A. R. Silva

Hi Sakari,

Please, see my comments below...

Quoted 77 lines · Sakari Ailus

Hi Gustavo,

Apologies for the delay.

Quoted 72 lines · Gustavo A. R. Silva

Hi Sakari,

Quoted 32 lines · Sakari Ailus

Hi Gustavo,

I missed you already had sent v2...

Quoted 27 lines · 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 *' and turn binary_header into a flexible-array member. Also, adjust the relational operator to be '<=' instead of '<', as it seems that the intention of the comparison is to determine if the loaded firmware contains any 'struct imgu_fw_info' items in the binary_header[] array than merely the file_header (struct imgu_fw_bi_file_h).

The replacement of the one-element array with a flexible-array member also help with the ongoing efforts to globally enable -Warray-bounds and get us closer to being able to tighten the FORTIFY_SOURCE routines on memcpy().

Link https://github.com/KSPP/linux/issues/79

Link https://github.com/KSPP/linux/issues/109

Fixes 09d290f0ba21 ("media: staging/intel-ipu3: css: Add support for firmware management")

Cc [email protected]

Signed-off-by Gustavo A. R. Silva <[email protected]>

---

It'd be just great if someone that knows this code better can confirm these changes are correct. In particular the adjustment of the relational operator. Thanks!

Quoted 5 lines

Quoted 2 lines

Changes in v2:
- Use flexible array and adjust relational operator, accordingly.

The operator was just correct. The check is just there to see the firmware is at least as large as the struct as which it is being accessed.

I'm a bit confused, so based on your reply to v1 of this series, this patch is now correct, right?

The operator in v1 _was_ correct as long as the one-element array wasn't transformed into a flexible array, right?

Notice that generally speaking flexible-array members don't occupy space in the containing structure:

$ pahole -C imgu_fw_header drivers/staging/media/ipu3/ipu3-css-fw.o
struct imgu_fw_header {
	struct imgu_fw_bi_file_h   file_header;          /*     0    72 */
	/* --- cacheline 1 boundary (64 bytes) was 8 bytes ago --- */
	struct imgu_fw_info        binary_header[] __attribute__((__aligned__(8))); /*    72     0 */

	/* size: 72, cachelines: 2, members: 2 */
	/* forced alignments: 1 */
	/* last cacheline: 8 bytes */
} __attribute__((__aligned__(8)));

$ pahole -C imgu_fw_header drivers/staging/media/ipu3/ipu3-css-fw.o
struct imgu_fw_header {
	struct imgu_fw_bi_file_h   file_header;          /*     0    72 */
	/* --- cacheline 1 boundary (64 bytes) was 8 bytes ago --- */
	struct imgu_fw_info        binary_header[1] __attribute__((__aligned__(8))); /*    72  1200 */

	/* size: 1272, cachelines: 20, members: 2 */
	/* forced alignments: 1 */
	/* last cacheline: 56 bytes */
} __attribute__((__aligned__(8)));

Quoted 7 lines

Quoted 2 lines

So, now that the flexible array transformation is included in the same patch as the bugfix, the operator is changed from '<' to '<='

'<' is correct since you only need as much data as the struct you're about to access is large, not a byte more than that. As Dan noted.

I think you could add a check for binary_nr is at least one.

If we need to check that binary_nr is at least one, then this would be the right change:

Review

Quoted 5 lines

        css->fwp = (struct imgu_fw_header *)css->fw->data;

(quoted diff) +1 -1

-       if (css->fw->size < sizeof(struct imgu_fw_header *) ||
+       if (css->fw->size < struct_size(css->fwp, binary_header, 1) ||
            css->fwp->file_header.h_size != sizeof(struct imgu_fw_bi_file_h))
                goto bad_fw;

There's already a check the space required for the array of binary_nr is there. But not the number itself.