greybus: Avoid fake flexible array for response data
Commit Message
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: Viresh Kumar <vireshk@kernel.org>
Cc: Johan Hovold <johan@kernel.org>
Cc: Alex Elder <elder@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
---
drivers/staging/greybus/bootrom.c | 7 +++----
drivers/staging/greybus/fw-download.c | 7 +++----
include/linux/greybus/greybus_protocols.h | 8 --------
3 files changed, 6 insertions(+), 16 deletions(-)
Comments
On 2/16/24 5:28 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.
The only down side I see is that it sort of disrupts a pattern
used on Greybus request handlers (and the response structure definitions).
I think a one-line comment in place of each of these two
definitions would be helpful, something like:
/* gb_fw_download_fetch_firmware_response contains no data */
And then add a similar comment above the calls to
gb_operation_response_alloc().
Otherwise this looks good.
Reviewed-by: Alex Elder <elder@linaro.org>
>
> Signed-off-by: Kees Cook <keescook@chromium.org>
> ---
> Cc: Viresh Kumar <vireshk@kernel.org>
> Cc: Johan Hovold <johan@kernel.org>
> Cc: Alex Elder <elder@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
> ---
> drivers/staging/greybus/bootrom.c | 7 +++----
> drivers/staging/greybus/fw-download.c | 7 +++----
> include/linux/greybus/greybus_protocols.h | 8 --------
> 3 files changed, 6 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/staging/greybus/bootrom.c b/drivers/staging/greybus/bootrom.c
> index 79581457c4af..76ad5f289072 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,14 @@ static int gb_bootrom_get_firmware(struct gb_operation *op)
> goto unlock;
> }
>
> - if (!gb_operation_response_alloc(op, sizeof(*firmware_response) + size,
> - GFP_KERNEL)) {
> + 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..2d73bb729aa2 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,7 @@ static int fw_download_fetch_firmware(struct gb_operation *op)
> goto put_fw;
> }
>
> - if (!gb_operation_response_alloc(op, sizeof(*response) + size,
> - GFP_KERNEL)) {
> + if (!gb_operation_response_alloc(op, size, GFP_KERNEL)) {
> dev_err(fw_download->parent,
> "error allocating fetch firmware response\n");
> ret = -ENOMEM;
> @@ -334,7 +333,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..603acdcc0c6b 100644
> --- a/include/linux/greybus/greybus_protocols.h
> +++ b/include/linux/greybus/greybus_protocols.h
> @@ -232,10 +232,6 @@ struct gb_fw_download_fetch_firmware_request {
> __le32 size;
> } __packed;
>
> -struct gb_fw_download_fetch_firmware_response {
> - __u8 data[0];
> -} __packed;
> -
> /* firmware download release firmware request */
> struct gb_fw_download_release_firmware_request {
> __u8 firmware_id;
> @@ -414,10 +410,6 @@ struct gb_bootrom_get_firmware_request {
> __le32 size;
> } __packed;
>
> -struct gb_bootrom_get_firmware_response {
> - __u8 data[0];
> -} __packed;
> -
> /* Bootrom protocol Ready to boot request */
> struct gb_bootrom_ready_to_boot_request {
> __u8 status;
On Sat, Feb 17, 2024 at 02:17:33PM -0600, Alex Elder wrote:
> On 2/16/24 5:28 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.
>
> The only down side I see is that it sort of disrupts a pattern
> used on Greybus request handlers (and the response structure definitions).
>
> I think a one-line comment in place of each of these two
> definitions would be helpful, something like:
> /* gb_fw_download_fetch_firmware_response contains no data */
Er, maybe this should be "no other data" ? Do you want a v2 of this
patch?
> And then add a similar comment above the calls to
> gb_operation_response_alloc().
>
> Otherwise this looks good.
>
> Reviewed-by: Alex Elder <elder@linaro.org>
Thanks!
On 2/17/24 3:58 PM, Kees Cook wrote:
> On Sat, Feb 17, 2024 at 02:17:33PM -0600, Alex Elder wrote:
>> On 2/16/24 5:28 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.
>>
>> The only down side I see is that it sort of disrupts a pattern
>> used on Greybus request handlers (and the response structure definitions).
>>
>> I think a one-line comment in place of each of these two
>> definitions would be helpful, something like:
>> /* gb_fw_download_fetch_firmware_response contains no data */
>
> Er, maybe this should be "no other data" ? Do you want a v2 of this
> patch?
Sending v2 is probably best, because I'd like to see these
comments. Greg could fix it up himself but he probably wants
to pull it from the list
And yes, "no other data" is fine, or maybe "no payload"
or "has an empty payload". Any of those is better than
nothing; you choose.
Thank you.
-Alex
>
>> And then add a similar comment above the calls to
>> gb_operation_response_alloc().
>>
>> Otherwise this looks good.
>>
>> Reviewed-by: Alex Elder <elder@linaro.org>
>
> Thanks!
>
@@ -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,14 @@ static int gb_bootrom_get_firmware(struct gb_operation *op)
goto unlock;
}
- if (!gb_operation_response_alloc(op, sizeof(*firmware_response) + size,
- GFP_KERNEL)) {
+ 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);
@@ -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,7 @@ static int fw_download_fetch_firmware(struct gb_operation *op)
goto put_fw;
}
- if (!gb_operation_response_alloc(op, sizeof(*response) + size,
- GFP_KERNEL)) {
+ if (!gb_operation_response_alloc(op, size, GFP_KERNEL)) {
dev_err(fw_download->parent,
"error allocating fetch firmware response\n");
ret = -ENOMEM;
@@ -334,7 +333,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,
@@ -232,10 +232,6 @@ struct gb_fw_download_fetch_firmware_request {
__le32 size;
} __packed;
-struct gb_fw_download_fetch_firmware_response {
- __u8 data[0];
-} __packed;
-
/* firmware download release firmware request */
struct gb_fw_download_release_firmware_request {
__u8 firmware_id;
@@ -414,10 +410,6 @@ struct gb_bootrom_get_firmware_request {
__le32 size;
} __packed;
-struct gb_bootrom_get_firmware_response {
- __u8 data[0];
-} __packed;
-
/* Bootrom protocol Ready to boot request */
struct gb_bootrom_ready_to_boot_request {
__u8 status;