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 --- Cc: Alex Elder elder@kernel.org Cc: Viresh Kumar vireshk@kernel.org Cc: Johan Hovold johan@kernel.org Cc: Greg Kroah-Hartman gregkh@linuxfoundation.org Cc: Gustavo A. R. Silva gustavo@embeddedor.com Cc: greybus-dev@lists.linaro.org Cc: linux-staging@lists.linux.dev v2: add comments about removed structs v1: https://patchwork.kernel.org/project/linux-hardening/patch/20240216232824.wo... --- drivers/staging/greybus/bootrom.c | 8 ++++---- drivers/staging/greybus/fw-download.c | 8 ++++---- include/linux/greybus/greybus_protocols.h | 8 ++------ 3 files changed, 10 insertions(+), 14 deletions(-)
diff --git a/drivers/staging/greybus/bootrom.c b/drivers/staging/greybus/bootrom.c index 79581457c4af..c0d338db6b52 100644 --- a/drivers/staging/greybus/bootrom.c +++ b/drivers/staging/greybus/bootrom.c @@ -243,10 +243,10 @@ static int gb_bootrom_get_firmware(struct gb_operation *op) struct gb_bootrom *bootrom = gb_connection_get_data(op->connection); const struct firmware *fw; struct gb_bootrom_get_firmware_request *firmware_request; - struct gb_bootrom_get_firmware_response *firmware_response; struct device *dev = &op->connection->bundle->dev; unsigned int offset, size; enum next_request_type next_request; + u8 *firmware_response; int ret = 0;
/* Disable timeouts */ @@ -280,15 +280,15 @@ static int gb_bootrom_get_firmware(struct gb_operation *op) goto unlock; }
- if (!gb_operation_response_alloc(op, sizeof(*firmware_response) + size, - GFP_KERNEL)) { + /* gb_bootrom_get_firmware_response contains only a byte array */ + if (!gb_operation_response_alloc(op, size, GFP_KERNEL)) { dev_err(dev, "%s: error allocating response\n", __func__); ret = -ENOMEM; goto unlock; }
firmware_response = op->response->payload; - memcpy(firmware_response->data, fw->data + offset, size); + memcpy(firmware_response, fw->data + offset, size);
dev_dbg(dev, "responding with firmware (offs = %u, size = %u)\n", offset, size); diff --git a/drivers/staging/greybus/fw-download.c b/drivers/staging/greybus/fw-download.c index 543692c567f9..a06065fb0c5e 100644 --- a/drivers/staging/greybus/fw-download.c +++ b/drivers/staging/greybus/fw-download.c @@ -271,11 +271,11 @@ static int fw_download_fetch_firmware(struct gb_operation *op) struct gb_connection *connection = op->connection; struct fw_download *fw_download = gb_connection_get_data(connection); struct gb_fw_download_fetch_firmware_request *request; - struct gb_fw_download_fetch_firmware_response *response; struct fw_request *fw_req; const struct firmware *fw; unsigned int offset, size; u8 firmware_id; + u8 *response; int ret = 0;
if (op->request->payload_size != sizeof(*request)) { @@ -325,8 +325,8 @@ static int fw_download_fetch_firmware(struct gb_operation *op) goto put_fw; }
- if (!gb_operation_response_alloc(op, sizeof(*response) + size, - GFP_KERNEL)) { + /* gb_fw_download_fetch_firmware_response contains only a byte array */ + if (!gb_operation_response_alloc(op, size, GFP_KERNEL)) { dev_err(fw_download->parent, "error allocating fetch firmware response\n"); ret = -ENOMEM; @@ -334,7 +334,7 @@ static int fw_download_fetch_firmware(struct gb_operation *op) }
response = op->response->payload; - memcpy(response->data, fw->data + offset, size); + memcpy(response, fw->data + offset, size);
dev_dbg(fw_download->parent, "responding with firmware (offs = %u, size = %u)\n", offset, diff --git a/include/linux/greybus/greybus_protocols.h b/include/linux/greybus/greybus_protocols.h index aeb8f9243545..820134b0105c 100644 --- a/include/linux/greybus/greybus_protocols.h +++ b/include/linux/greybus/greybus_protocols.h @@ -232,9 +232,7 @@ struct gb_fw_download_fetch_firmware_request { __le32 size; } __packed;
-struct gb_fw_download_fetch_firmware_response { - __u8 data[0]; -} __packed; +/* gb_fw_download_fetch_firmware_response contains no other data */
/* firmware download release firmware request */ struct gb_fw_download_release_firmware_request { @@ -414,9 +412,7 @@ struct gb_bootrom_get_firmware_request { __le32 size; } __packed;
-struct gb_bootrom_get_firmware_response { - __u8 data[0]; -} __packed; +/* gb_bootrom_get_firmware_response contains no other data */
/* Bootrom protocol Ready to boot request */ struct gb_bootrom_ready_to_boot_request {
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.
-Alex
Cc: Alex Elder elder@kernel.org Cc: Viresh Kumar vireshk@kernel.org Cc: Johan Hovold johan@kernel.org Cc: Greg Kroah-Hartman gregkh@linuxfoundation.org Cc: Gustavo A. R. Silva gustavo@embeddedor.com Cc: greybus-dev@lists.linaro.org Cc: linux-staging@lists.linux.dev v2: add comments about removed structs v1: https://patchwork.kernel.org/project/linux-hardening/patch/20240216232824.wo...
drivers/staging/greybus/bootrom.c | 8 ++++---- drivers/staging/greybus/fw-download.c | 8 ++++---- include/linux/greybus/greybus_protocols.h | 8 ++------ 3 files changed, 10 insertions(+), 14 deletions(-)
diff --git a/drivers/staging/greybus/bootrom.c b/drivers/staging/greybus/bootrom.c index 79581457c4af..c0d338db6b52 100644 --- a/drivers/staging/greybus/bootrom.c +++ b/drivers/staging/greybus/bootrom.c @@ -243,10 +243,10 @@ static int gb_bootrom_get_firmware(struct gb_operation *op) struct gb_bootrom *bootrom = gb_connection_get_data(op->connection); const struct firmware *fw; struct gb_bootrom_get_firmware_request *firmware_request;
- struct gb_bootrom_get_firmware_response *firmware_response; struct device *dev = &op->connection->bundle->dev; unsigned int offset, size; enum next_request_type next_request;
- u8 *firmware_response; int ret = 0;
/* Disable timeouts */ @@ -280,15 +280,15 @@ static int gb_bootrom_get_firmware(struct gb_operation *op) goto unlock; }
- if (!gb_operation_response_alloc(op, sizeof(*firmware_response) + size,
GFP_KERNEL)) {
- /* gb_bootrom_get_firmware_response contains only a byte array */
- if (!gb_operation_response_alloc(op, size, GFP_KERNEL)) { dev_err(dev, "%s: error allocating response\n", __func__); ret = -ENOMEM; goto unlock; }
firmware_response = op->response->payload;
- memcpy(firmware_response->data, fw->data + offset, size);
- memcpy(firmware_response, fw->data + offset, size);
dev_dbg(dev, "responding with firmware (offs = %u, size = %u)\n", offset, size); diff --git a/drivers/staging/greybus/fw-download.c b/drivers/staging/greybus/fw-download.c index 543692c567f9..a06065fb0c5e 100644 --- a/drivers/staging/greybus/fw-download.c +++ b/drivers/staging/greybus/fw-download.c @@ -271,11 +271,11 @@ static int fw_download_fetch_firmware(struct gb_operation *op) struct gb_connection *connection = op->connection; struct fw_download *fw_download = gb_connection_get_data(connection); struct gb_fw_download_fetch_firmware_request *request;
- struct gb_fw_download_fetch_firmware_response *response; struct fw_request *fw_req; const struct firmware *fw; unsigned int offset, size; u8 firmware_id;
- u8 *response; int ret = 0;
if (op->request->payload_size != sizeof(*request)) { @@ -325,8 +325,8 @@ static int fw_download_fetch_firmware(struct gb_operation *op) goto put_fw; }
- if (!gb_operation_response_alloc(op, sizeof(*response) + size,
GFP_KERNEL)) {
- /* gb_fw_download_fetch_firmware_response contains only a byte array */
- if (!gb_operation_response_alloc(op, size, GFP_KERNEL)) { dev_err(fw_download->parent, "error allocating fetch firmware response\n"); ret = -ENOMEM;
@@ -334,7 +334,7 @@ static int fw_download_fetch_firmware(struct gb_operation *op) } response = op->response->payload;
- memcpy(response->data, fw->data + offset, size);
- memcpy(response, fw->data + offset, size);
dev_dbg(fw_download->parent, "responding with firmware (offs = %u, size = %u)\n", offset, diff --git a/include/linux/greybus/greybus_protocols.h b/include/linux/greybus/greybus_protocols.h index aeb8f9243545..820134b0105c 100644 --- a/include/linux/greybus/greybus_protocols.h +++ b/include/linux/greybus/greybus_protocols.h @@ -232,9 +232,7 @@ struct gb_fw_download_fetch_firmware_request { __le32 size; } __packed; -struct gb_fw_download_fetch_firmware_response {
- __u8 data[0];
-} __packed; +/* gb_fw_download_fetch_firmware_response contains no other data */ /* firmware download release firmware request */ struct gb_fw_download_release_firmware_request { @@ -414,9 +412,7 @@ struct gb_bootrom_get_firmware_request { __le32 size; } __packed; -struct gb_bootrom_get_firmware_response {
- __u8 data[0];
-} __packed; +/* gb_bootrom_get_firmware_response contains no other data */ /* Bootrom protocol Ready to boot request */ struct gb_bootrom_ready_to_boot_request {
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
-
Alex Elder -
Kees Cook