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@xxxxxxxxxxxxxxx >>>>>> Signed-off-by: Gustavo A. R. Silva <gustavoars@xxxxxxxxxx> >>>>>> --- >>>>>> >>>>>> 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
