On Mon, Mar 04, 2024 at 04:45:11PM -0600, Alex Elder wrote: > On 3/4/24 3:19 PM, Kees Cook wrote: > > FORTIFY_SOURCE has been ignoring 0-sized destinations while the kernel > > code base has been converted to flexible arrays. In order to enforce > > the 0-sized destinations (e.g. with __counted_by), the remaining 0-sized > > destinations need to be handled. Instead of converting an empty struct > > into using a flexible array, just directly use a pointer without any > > additional indirection. Remove struct gb_bootrom_get_firmware_response > > and struct gb_fw_download_fetch_firmware_response. > > > > Signed-off-by: Kees Cook <keescook@xxxxxxxxxxxx> > > Thanks for adding the comments! This looks good to me. > > Reviewed-by: Alex Elder <elder@xxxxxxxxxx> > > > > I want to call attention to a few other spots that should > get a little more attention--related directly to what you're > doing here. > > I noticed that the GB_CONTROL_TYPE_GET_MANIFEST response > structure also contains only a flexible array. It might > be good to add a similar comment in gb_interface_enable(), > above this line: > manifest = kmalloc(size, GFP_KERNEL); > The definition of the gb_control_get_manifest_response structure > could probably be replaced with a comment. > > > The response buffer for an I2C transfer consists only of incoming > data. There is already a comment in gb_i2c_operation_create() > that says this: > /* Response consists only of incoming data */ > The definition of the gb_i2c_transfer_response structure should > then go away, in favor of a comment saying this. > > The response buffer for a SPI transfer consists only of incoming > data. It is used three times in "driver/staging/greybus/spilib.c": > - calc_rx_xfer_size() subtracts the size of the response structure, > and that should be replaced by a comment (and the structure > definition should go away) > - gb_spi_decode_response() takes the response structure as an > argument. That could be replaced with a void pointer instead, > with a comment. > - gb_spi_transfer_one_message() is what passes the response buffer > to gb_spi_decode_response(), and could be adjusted to reflect > that the response consists only of data--rather than a struct > containing only a flexible array. > > > Kees: I'm *not* asking you to deal with these, I'm just mentioning > them to you. My comments above (without someone else confirming) > are not sufficient to dictate how to address these. Okay, thanks! Yeah, I took a look at struct gb_i2c_transfer_response and I think it might trip the memcpy checking too since it's zero sized, but it's on the source side, which isn't as strictly checked. I'll add a TODO item to track these, though. -Kees -- Kees Cook
