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@chromium.org
Thanks for adding the comments! This looks good to me.
Reviewed-by: Alex Elder elder@linaro.org
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