linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v2 0/2] Fix size comparison bug and use flexible array
@ 2021-07-30 12:07 Gustavo A. R. Silva
  2021-07-30 12:08 ` [PATCH v2 1/2] media: staging/intel-ipu3: css: Fix wrong size comparison Gustavo A. R. Silva
  2021-07-30 12:09 ` [PATCH v2 2/2] media: staging/intel-ipu3: css: Use the struct_size() helper Gustavo A. R. Silva
  0 siblings, 2 replies; 10+ messages in thread
From: Gustavo A. R. Silva @ 2021-07-30 12:07 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,
	Dan Carpenter

Hi all,

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

Thanks

Changes in v2:
 - Use flexible array and adjust relational operator in patch 1.

Gustavo A. R. Silva (2):
  media: staging/intel-ipu3: css: Fix wrong size comparison
  media: staging/intel-ipu3: css: Use the 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] 10+ messages in thread

* [PATCH v2 1/2] media: staging/intel-ipu3: css: Fix wrong size comparison
  2021-07-30 12:07 [PATCH v2 0/2] Fix size comparison bug and use flexible array Gustavo A. R. Silva
@ 2021-07-30 12:08 ` Gustavo A. R. Silva
  2021-08-02  6:05   ` Sakari Ailus
  2021-07-30 12:09 ` [PATCH v2 2/2] media: staging/intel-ipu3: css: Use the struct_size() helper Gustavo A. R. Silva
  1 sibling, 1 reply; 10+ messages in thread
From: Gustavo A. R. Silva @ 2021-07-30 12:08 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,
	Dan Carpenter

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: stable@vger.kernel.org
Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org>
---

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(-)

diff --git a/drivers/staging/media/ipu3/ipu3-css-fw.c b/drivers/staging/media/ipu3/ipu3-css-fw.c
index 45aff76198e2..630cb5186b48 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) +
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] 10+ messages in thread

* [PATCH v2 2/2] media: staging/intel-ipu3: css: Use the struct_size() helper
  2021-07-30 12:07 [PATCH v2 0/2] Fix size comparison bug and use flexible array Gustavo A. R. Silva
  2021-07-30 12:08 ` [PATCH v2 1/2] media: staging/intel-ipu3: css: Fix wrong size comparison Gustavo A. R. Silva
@ 2021-07-30 12:09 ` Gustavo A. R. Silva
  1 sibling, 0 replies; 10+ messages in thread
From: Gustavo A. R. Silva @ 2021-07-30 12:09 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,
	Dan Carpenter

Make use of the struct_size() helper instead of an open-coded version.

Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org>
---
Changes in v2:
 - Move the replacement of one-element array with a flexible-array
   member to patch 1 of the series and update changelog text,
   accordingly.

 drivers/staging/media/ipu3/ipu3-css-fw.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/staging/media/ipu3/ipu3-css-fw.c b/drivers/staging/media/ipu3/ipu3-css-fw.c
index 630cb5186b48..b9c850fc9fe4 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",
-- 
2.27.0


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

* Re: [PATCH v2 1/2] media: staging/intel-ipu3: css: Fix wrong size comparison
  2021-07-30 12:08 ` [PATCH v2 1/2] media: staging/intel-ipu3: css: Fix wrong size comparison Gustavo A. R. Silva
@ 2021-08-02  6:05   ` Sakari Ailus
  2021-08-02 13:46     ` Gustavo A. R. Silva
  0 siblings, 1 reply; 10+ messages in thread
From: Sakari Ailus @ 2021-08-02  6:05 UTC (permalink / raw)
  To: Gustavo A. R. Silva
  Cc: linux-kernel, Yong Zhi, Bingbu Cao, Tianshu Qiu,
	Mauro Carvalho Chehab, Greg Kroah-Hartman, linux-media,
	linux-staging, linux-hardening, Dan Carpenter

Hi Gustavo,

I missed you already had sent v2...

On Fri, Jul 30, 2021 at 07:08:13AM -0500, Gustavo A. R. Silva wrote:
> 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: stable@vger.kernel.org
> Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org>
> ---
> 
> 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.

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.

>  - 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(-)
> 
> diff --git a/drivers/staging/media/ipu3/ipu3-css-fw.c b/drivers/staging/media/ipu3/ipu3-css-fw.c
> index 45aff76198e2..630cb5186b48 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) +
> 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 *******************/

-- 
Regards,

Sakari Ailus

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

* Re: [PATCH v2 1/2] media: staging/intel-ipu3: css: Fix wrong size comparison
  2021-08-02  6:05   ` Sakari Ailus
@ 2021-08-02 13:46     ` Gustavo A. R. Silva
  2021-08-04 12:37       ` Dan Carpenter
  2021-08-10 15:18       ` Sakari Ailus
  0 siblings, 2 replies; 10+ messages in thread
From: Gustavo A. R. Silva @ 2021-08-02 13:46 UTC (permalink / raw)
  To: Sakari Ailus, Gustavo A. R. Silva
  Cc: linux-kernel, Yong Zhi, Bingbu Cao, Tianshu Qiu,
	Mauro Carvalho Chehab, Greg Kroah-Hartman, linux-media,
	linux-staging, linux-hardening, Dan Carpenter

Hi Sakari,

On 8/2/21 01:05, Sakari Ailus wrote:
> Hi Gustavo,
> 
> I missed you already had sent v2...
> 
> On Fri, Jul 30, 2021 at 07:08:13AM -0500, Gustavo A. R. Silva wrote:
>> 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: stable@vger.kernel.org
>> Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org>
>> ---
>>
>> 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.
> 
> 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 '<='

Can you please confirm if this v2 is now correct? if so, it'd be great to have your
Reviewed-by tag. :)

Thanks!
--
Gustavo

> 
>>  - 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(-)
>>
>> diff --git a/drivers/staging/media/ipu3/ipu3-css-fw.c b/drivers/staging/media/ipu3/ipu3-css-fw.c
>> index 45aff76198e2..630cb5186b48 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) +
>> 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 *******************/
> 

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

* Re: [PATCH v2 1/2] media: staging/intel-ipu3: css: Fix wrong size comparison
  2021-08-02 13:46     ` Gustavo A. R. Silva
@ 2021-08-04 12:37       ` Dan Carpenter
  2021-08-10 15:18       ` Sakari Ailus
  1 sibling, 0 replies; 10+ messages in thread
From: Dan Carpenter @ 2021-08-04 12:37 UTC (permalink / raw)
  To: Gustavo A. R. Silva
  Cc: Sakari Ailus, Gustavo A. R. Silva, linux-kernel, Yong Zhi,
	Bingbu Cao, Tianshu Qiu, Mauro Carvalho Chehab,
	Greg Kroah-Hartman, linux-media, linux-staging, linux-hardening

I missed that you change < sizeof() to <= sizeof()...  Sakari is right
that it should be < sizeof().  <= doesn't work at all ever.  If you
wanted a higher limit then the next limit woule be:

	if (css->fw->size < struct_size(css->fwp, binary_header, 1))

regards,
dan carpenter


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

* Re: [PATCH v2 1/2] media: staging/intel-ipu3: css: Fix wrong size comparison
  2021-08-02 13:46     ` Gustavo A. R. Silva
  2021-08-04 12:37       ` Dan Carpenter
@ 2021-08-10 15:18       ` Sakari Ailus
  2021-08-10 16:26         ` Gustavo A. R. Silva
  1 sibling, 1 reply; 10+ messages in thread
From: Sakari Ailus @ 2021-08-10 15:18 UTC (permalink / raw)
  To: Gustavo A. R. Silva
  Cc: Gustavo A. R. Silva, linux-kernel, Yong Zhi, Bingbu Cao,
	Tianshu Qiu, Mauro Carvalho Chehab, Greg Kroah-Hartman,
	linux-media, linux-staging, linux-hardening, Dan Carpenter

Hi Gustavo,

Apologies for the delay.

On Mon, Aug 02, 2021 at 08:46:20AM -0500, Gustavo A. R. Silva wrote:
> Hi Sakari,
> 
> On 8/2/21 01:05, Sakari Ailus wrote:
> > Hi Gustavo,
> > 
> > I missed you already had sent v2...
> > 
> > On Fri, Jul 30, 2021 at 07:08:13AM -0500, Gustavo A. R. Silva wrote:
> >> 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: stable@vger.kernel.org
> >> Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org>
> >> ---
> >>
> >> 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.
> > 
> > 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 '<='

'<' 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.

-- 
Kind regards,

Sakari Ailus

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

* Re: [PATCH v2 1/2] media: staging/intel-ipu3: css: Fix wrong size comparison
  2021-08-10 15:18       ` Sakari Ailus
@ 2021-08-10 16:26         ` Gustavo A. R. Silva
  2021-08-10 16:30           ` Sakari Ailus
  0 siblings, 1 reply; 10+ messages in thread
From: Gustavo A. R. Silva @ 2021-08-10 16:26 UTC (permalink / raw)
  To: Sakari Ailus
  Cc: Gustavo A. R. Silva, linux-kernel, Yong Zhi, Bingbu Cao,
	Tianshu Qiu, Mauro Carvalho Chehab, Greg Kroah-Hartman,
	linux-media, linux-staging, linux-hardening, Dan Carpenter

Hi Sakari,

Please, see my comments below...

On 8/10/21 10:18, Sakari Ailus wrote:
> Hi Gustavo,
> 
> Apologies for the delay.
> 
> On Mon, Aug 02, 2021 at 08:46:20AM -0500, Gustavo A. R. Silva wrote:
>> Hi Sakari,
>>
>> On 8/2/21 01:05, Sakari Ailus wrote:
>>> Hi Gustavo,
>>>
>>> I missed you already had sent v2...
>>>
>>> On Fri, Jul 30, 2021 at 07:08:13AM -0500, Gustavo A. R. Silva wrote:
>>>> 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: stable@vger.kernel.org
>>>> Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org>
>>>> ---
>>>>
>>>> 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.
>>>
>>> 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 '<='
> 
> '<' 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;
-       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;


--
Gustavo

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

* Re: [PATCH v2 1/2] media: staging/intel-ipu3: css: Fix wrong size comparison
  2021-08-10 16:26         ` Gustavo A. R. Silva
@ 2021-08-10 16:30           ` Sakari Ailus
  2021-08-10 16:40             ` Gustavo A. R. Silva
  0 siblings, 1 reply; 10+ messages in thread
From: Sakari Ailus @ 2021-08-10 16:30 UTC (permalink / raw)
  To: Gustavo A. R. Silva
  Cc: Gustavo A. R. Silva, linux-kernel, Yong Zhi, Bingbu Cao,
	Tianshu Qiu, Mauro Carvalho Chehab, Greg Kroah-Hartman,
	linux-media, linux-staging, linux-hardening, Dan Carpenter

On Tue, Aug 10, 2021 at 11:26:14AM -0500, Gustavo A. R. Silva wrote:
> Hi Sakari,
> 
> Please, see my comments below...
> 
> On 8/10/21 10:18, Sakari Ailus wrote:
> > Hi Gustavo,
> > 
> > Apologies for the delay.
> > 
> > On Mon, Aug 02, 2021 at 08:46:20AM -0500, Gustavo A. R. Silva wrote:
> >> Hi Sakari,
> >>
> >> On 8/2/21 01:05, Sakari Ailus wrote:
> >>> Hi Gustavo,
> >>>
> >>> I missed you already had sent v2...
> >>>
> >>> On Fri, Jul 30, 2021 at 07:08:13AM -0500, Gustavo A. R. Silva wrote:
> >>>> 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: stable@vger.kernel.org
> >>>> Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org>
> >>>> ---
> >>>>
> >>>> 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.
> >>>
> >>> 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 '<='
> > 
> > '<' 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;
> -       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.

-- 
Sakari Ailus

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

* Re: [PATCH v2 1/2] media: staging/intel-ipu3: css: Fix wrong size comparison
  2021-08-10 16:30           ` Sakari Ailus
@ 2021-08-10 16:40             ` Gustavo A. R. Silva
  0 siblings, 0 replies; 10+ messages in thread
From: Gustavo A. R. Silva @ 2021-08-10 16:40 UTC (permalink / raw)
  To: Sakari Ailus
  Cc: Gustavo A. R. Silva, linux-kernel, Yong Zhi, Bingbu Cao,
	Tianshu Qiu, Mauro Carvalho Chehab, Greg Kroah-Hartman,
	linux-media, linux-staging, linux-hardening, Dan Carpenter



On 8/10/21 11:30, Sakari Ailus wrote:
> On Tue, Aug 10, 2021 at 11:26:14AM -0500, Gustavo A. R. Silva wrote:
>> Hi Sakari,
>>
>> Please, see my comments below...
>>
>> On 8/10/21 10:18, Sakari Ailus wrote:
>>> Hi Gustavo,
>>>
>>> Apologies for the delay.
>>>
>>> On Mon, Aug 02, 2021 at 08:46:20AM -0500, Gustavo A. R. Silva wrote:
>>>> Hi Sakari,
>>>>
>>>> On 8/2/21 01:05, Sakari Ailus wrote:
>>>>> Hi Gustavo,
>>>>>
>>>>> I missed you already had sent v2...
>>>>>
>>>>> On Fri, Jul 30, 2021 at 07:08:13AM -0500, Gustavo A. R. Silva wrote:
>>>>>> 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: stable@vger.kernel.org
>>>>>> Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org>
>>>>>> ---
>>>>>>
>>>>>> 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.
>>>>>
>>>>> 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 '<='
>>>
>>> '<' 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;
>> -       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.

Yep; and that is for the upper limit.

The whole fix would be this:

diff --git a/drivers/staging/media/ipu3/ipu3-css-fw.c b/drivers/staging/media/ipu3/ipu3-css-fw.c
index 45aff76198e2..8830f42f2b12 100644
--- a/drivers/staging/media/ipu3/ipu3-css-fw.c
+++ b/drivers/staging/media/ipu3/ipu3-css-fw.c
@@ -124,12 +124,11 @@ 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 < struct_size(css->fwp, binary_header, 1) ||
            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((struct imgu_fw_header *)0, 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 *******************/


Notice that "css->fw->size < struct_size(css->fwp, binary_header, 1)" with binary_header declared as a flexible-array
member is equivalent to "css->fw->size < sizeof(struct imgu_fw_header)" with binary_header declared as a one-element
array.

--
Gustavo

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

end of thread, other threads:[~2021-08-10 16:44 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-07-30 12:07 [PATCH v2 0/2] Fix size comparison bug and use flexible array Gustavo A. R. Silva
2021-07-30 12:08 ` [PATCH v2 1/2] media: staging/intel-ipu3: css: Fix wrong size comparison Gustavo A. R. Silva
2021-08-02  6:05   ` Sakari Ailus
2021-08-02 13:46     ` Gustavo A. R. Silva
2021-08-04 12:37       ` Dan Carpenter
2021-08-10 15:18       ` Sakari Ailus
2021-08-10 16:26         ` Gustavo A. R. Silva
2021-08-10 16:30           ` Sakari Ailus
2021-08-10 16:40             ` Gustavo A. R. Silva
2021-07-30 12:09 ` [PATCH v2 2/2] media: staging/intel-ipu3: css: Use the struct_size() helper Gustavo A. R. Silva

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).