[PATCH v2] greybus: Avoid fake flexible array for response data

[PATCH v2] greybus: Avoid fake flexible array for response data

[clingfei]

We want to get rid of zero size arrays and use flexible arrays instead.
However, in this case the struct is just one flexible array of u8 which
adds no value. Just use a pointer instead.

v1: https://lore.kernel.org/all/202505262032.507AD8E0DC@keescook/

Signed-off-by: clingfei <clf700383@gmail.com>
---
 drivers/staging/greybus/i2c.c             | 12 ++++--------
 include/linux/greybus/greybus_protocols.h |  3 ---
 2 files changed, 4 insertions(+), 11 deletions(-)

diff --git a/drivers/staging/greybus/i2c.c b/drivers/staging/greybus/i2c.c
index 14f1ff6d448c..b248d6717b71 100644
--- a/drivers/staging/greybus/i2c.c
+++ b/drivers/staging/greybus/i2c.c
@@ -144,15 +144,14 @@ gb_i2c_operation_create(struct gb_connection *connection,
 }
 
 static void gb_i2c_decode_response(struct i2c_msg *msgs, u32 msg_count,
-				   struct gb_i2c_transfer_response *response)
+				   u8 *data)
 {
 	struct i2c_msg *msg = msgs;
-	u8 *data;
 	u32 i;
 
-	if (!response)
+	if (!data)
 		return;
-	data = response->data;
+
 	for (i = 0; i < msg_count; i++) {
 		if (msg->flags & I2C_M_RD) {
 			memcpy(msg->buf, data, msg->len);
@@ -188,10 +187,7 @@ static int gb_i2c_transfer_operation(struct gb_i2c_device *gb_i2c_dev,
 
 	ret = gb_operation_request_send_sync(operation);
 	if (!ret) {
-		struct gb_i2c_transfer_response *response;
-
-		response = operation->response->payload;
-		gb_i2c_decode_response(msgs, msg_count, response);
+		gb_i2c_decode_response(msgs, msg_count, operation->response->payload);
 		ret = msg_count;
 	} else if (!gb_i2c_expected_transfer_error(ret)) {
 		dev_err(dev, "transfer operation failed (%d)\n", ret);
diff --git a/include/linux/greybus/greybus_protocols.h b/include/linux/greybus/greybus_protocols.h
index 820134b0105c..6a35c78b967b 100644
--- a/include/linux/greybus/greybus_protocols.h
+++ b/include/linux/greybus/greybus_protocols.h
@@ -678,9 +678,6 @@ struct gb_i2c_transfer_request {
 	__le16				op_count;
 	struct gb_i2c_transfer_op	ops[];		/* op_count of these */
 } __packed;
-struct gb_i2c_transfer_response {
-	__u8				data[0];	/* inbound data */
-} __packed;
 
 
 /* GPIO */
-- 
2.34.1

Re: [PATCH v2] greybus: Avoid fake flexible array for response data

[Greg KH]

On Tue, May 27, 2025 at 01:06:35PM +0800, clingfei wrote:
> We want to get rid of zero size arrays and use flexible arrays instead.
> However, in this case the struct is just one flexible array of u8 which
> adds no value. Just use a pointer instead.

Not true at all, sorry, it does "add value".  Please read the greybus
specification if you have questions about this.

> 
> v1: https://lore.kernel.org/all/202505262032.507AD8E0DC@keescook/

Please read our documentation for how to properly version kernel patches

> 
> Signed-off-by: clingfei <clf700383@gmail.com>

Also, we need a "full"name, not an email alias.

> ---
>  drivers/staging/greybus/i2c.c             | 12 ++++--------
>  include/linux/greybus/greybus_protocols.h |  3 ---
>  2 files changed, 4 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/staging/greybus/i2c.c b/drivers/staging/greybus/i2c.c
> index 14f1ff6d448c..b248d6717b71 100644
> --- a/drivers/staging/greybus/i2c.c
> +++ b/drivers/staging/greybus/i2c.c
> @@ -144,15 +144,14 @@ gb_i2c_operation_create(struct gb_connection *connection,
>  }
>  
>  static void gb_i2c_decode_response(struct i2c_msg *msgs, u32 msg_count,
> -				   struct gb_i2c_transfer_response *response)
> +				   u8 *data)
>  {
>  	struct i2c_msg *msg = msgs;
> -	u8 *data;
>  	u32 i;
>  
> -	if (!response)
> +	if (!data)
>  		return;
> -	data = response->data;
> +
>  	for (i = 0; i < msg_count; i++) {
>  		if (msg->flags & I2C_M_RD) {
>  			memcpy(msg->buf, data, msg->len);
> @@ -188,10 +187,7 @@ static int gb_i2c_transfer_operation(struct gb_i2c_device *gb_i2c_dev,
>  
>  	ret = gb_operation_request_send_sync(operation);
>  	if (!ret) {
> -		struct gb_i2c_transfer_response *response;
> -
> -		response = operation->response->payload;
> -		gb_i2c_decode_response(msgs, msg_count, response);
> +		gb_i2c_decode_response(msgs, msg_count, operation->response->payload);
>  		ret = msg_count;
>  	} else if (!gb_i2c_expected_transfer_error(ret)) {
>  		dev_err(dev, "transfer operation failed (%d)\n", ret);
> diff --git a/include/linux/greybus/greybus_protocols.h b/include/linux/greybus/greybus_protocols.h
> index 820134b0105c..6a35c78b967b 100644
> --- a/include/linux/greybus/greybus_protocols.h
> +++ b/include/linux/greybus/greybus_protocols.h
> @@ -678,9 +678,6 @@ struct gb_i2c_transfer_request {
>  	__le16				op_count;
>  	struct gb_i2c_transfer_op	ops[];		/* op_count of these */
>  } __packed;
> -struct gb_i2c_transfer_response {
> -	__u8				data[0];	/* inbound data */
> -} __packed;

As I said before, you can't just delete structures that are exported to
userspace without breaking things.  Why is this change acceptable to do
that?

And how was any of this tested?

greg k-h

Re: [PATCH v2] greybus: Avoid fake flexible array for response data

[clingfei]

Greg KH <gregkh@linuxfoundation.org> 于2025年5月27日周二 13:15写道：
>
> On Tue, May 27, 2025 at 01:06:35PM +0800, clingfei wrote:
> > We want to get rid of zero size arrays and use flexible arrays instead.
> > However, in this case the struct is just one flexible array of u8 which
> > adds no value. Just use a pointer instead.
>
> Not true at all, sorry, it does "add value".  Please read the greybus
> specification if you have questions about this.
>
> >
> > v1: https://lore.kernel.org/all/202505262032.507AD8E0DC@keescook/
>
> Please read our documentation for how to properly version kernel patches

Sorry, I will read it.
>
> >
> > Signed-off-by: clingfei <clf700383@gmail.com>
>
> Also, we need a "full"name, not an email alias.
>
> > ---
> >  drivers/staging/greybus/i2c.c             | 12 ++++--------
> >  include/linux/greybus/greybus_protocols.h |  3 ---
> >  2 files changed, 4 insertions(+), 11 deletions(-)
> >
> > diff --git a/drivers/staging/greybus/i2c.c b/drivers/staging/greybus/i2c.c
> > index 14f1ff6d448c..b248d6717b71 100644
> > --- a/drivers/staging/greybus/i2c.c
> > +++ b/drivers/staging/greybus/i2c.c
> > @@ -144,15 +144,14 @@ gb_i2c_operation_create(struct gb_connection *connection,
> >  }
> >
> >  static void gb_i2c_decode_response(struct i2c_msg *msgs, u32 msg_count,
> > -                                struct gb_i2c_transfer_response *response)
> > +                                u8 *data)
> >  {
> >       struct i2c_msg *msg = msgs;
> > -     u8 *data;
> >       u32 i;
> >
> > -     if (!response)
> > +     if (!data)
> >               return;
> > -     data = response->data;
> > +
> >       for (i = 0; i < msg_count; i++) {
> >               if (msg->flags & I2C_M_RD) {
> >                       memcpy(msg->buf, data, msg->len);
> > @@ -188,10 +187,7 @@ static int gb_i2c_transfer_operation(struct gb_i2c_device *gb_i2c_dev,
> >
> >       ret = gb_operation_request_send_sync(operation);
> >       if (!ret) {
> > -             struct gb_i2c_transfer_response *response;
> > -
> > -             response = operation->response->payload;
> > -             gb_i2c_decode_response(msgs, msg_count, response);
> > +             gb_i2c_decode_response(msgs, msg_count, operation->response->payload);
> >               ret = msg_count;
> >       } else if (!gb_i2c_expected_transfer_error(ret)) {
> >               dev_err(dev, "transfer operation failed (%d)\n", ret);
> > diff --git a/include/linux/greybus/greybus_protocols.h b/include/linux/greybus/greybus_protocols.h
> > index 820134b0105c..6a35c78b967b 100644
> > --- a/include/linux/greybus/greybus_protocols.h
> > +++ b/include/linux/greybus/greybus_protocols.h
> > @@ -678,9 +678,6 @@ struct gb_i2c_transfer_request {
> >       __le16                          op_count;
> >       struct gb_i2c_transfer_op       ops[];          /* op_count of these */
> >  } __packed;
> > -struct gb_i2c_transfer_response {
> > -     __u8                            data[0];        /* inbound data */
> > -} __packed;
>
> As I said before, you can't just delete structures that are exported to
> userspace without breaking things.  Why is this change acceptable to do
> that?
>
> And how was any of this tested?
>
> greg k-h

Could you please give some examples that will be broken by this change?
And I am not sure how this should be tested. It seems that it will not
have any negative impact on functionality.

Re: [PATCH v2] greybus: Avoid fake flexible array for response data

[Greg KH]

On Tue, May 27, 2025 at 01:50:42PM +0800, clingfei wrote:
> Greg KH <gregkh@linuxfoundation.org> 于2025年5月27日周二 13:15写道：
> >
> > On Tue, May 27, 2025 at 01:06:35PM +0800, clingfei wrote:
> > > We want to get rid of zero size arrays and use flexible arrays instead.
> > > However, in this case the struct is just one flexible array of u8 which
> > > adds no value. Just use a pointer instead.
> >
> > Not true at all, sorry, it does "add value".  Please read the greybus
> > specification if you have questions about this.
> >
> > >
> > > v1: https://lore.kernel.org/all/202505262032.507AD8E0DC@keescook/
> >
> > Please read our documentation for how to properly version kernel patches
> 
> Sorry, I will read it.
> >
> > >
> > > Signed-off-by: clingfei <clf700383@gmail.com>
> >
> > Also, we need a "full"name, not an email alias.
> >
> > > ---
> > >  drivers/staging/greybus/i2c.c             | 12 ++++--------
> > >  include/linux/greybus/greybus_protocols.h |  3 ---
> > >  2 files changed, 4 insertions(+), 11 deletions(-)
> > >
> > > diff --git a/drivers/staging/greybus/i2c.c b/drivers/staging/greybus/i2c.c
> > > index 14f1ff6d448c..b248d6717b71 100644
> > > --- a/drivers/staging/greybus/i2c.c
> > > +++ b/drivers/staging/greybus/i2c.c
> > > @@ -144,15 +144,14 @@ gb_i2c_operation_create(struct gb_connection *connection,
> > >  }
> > >
> > >  static void gb_i2c_decode_response(struct i2c_msg *msgs, u32 msg_count,
> > > -                                struct gb_i2c_transfer_response *response)
> > > +                                u8 *data)
> > >  {
> > >       struct i2c_msg *msg = msgs;
> > > -     u8 *data;
> > >       u32 i;
> > >
> > > -     if (!response)
> > > +     if (!data)
> > >               return;
> > > -     data = response->data;
> > > +
> > >       for (i = 0; i < msg_count; i++) {
> > >               if (msg->flags & I2C_M_RD) {
> > >                       memcpy(msg->buf, data, msg->len);
> > > @@ -188,10 +187,7 @@ static int gb_i2c_transfer_operation(struct gb_i2c_device *gb_i2c_dev,
> > >
> > >       ret = gb_operation_request_send_sync(operation);
> > >       if (!ret) {
> > > -             struct gb_i2c_transfer_response *response;
> > > -
> > > -             response = operation->response->payload;
> > > -             gb_i2c_decode_response(msgs, msg_count, response);
> > > +             gb_i2c_decode_response(msgs, msg_count, operation->response->payload);
> > >               ret = msg_count;
> > >       } else if (!gb_i2c_expected_transfer_error(ret)) {
> > >               dev_err(dev, "transfer operation failed (%d)\n", ret);
> > > diff --git a/include/linux/greybus/greybus_protocols.h b/include/linux/greybus/greybus_protocols.h
> > > index 820134b0105c..6a35c78b967b 100644
> > > --- a/include/linux/greybus/greybus_protocols.h
> > > +++ b/include/linux/greybus/greybus_protocols.h
> > > @@ -678,9 +678,6 @@ struct gb_i2c_transfer_request {
> > >       __le16                          op_count;
> > >       struct gb_i2c_transfer_op       ops[];          /* op_count of these */
> > >  } __packed;
> > > -struct gb_i2c_transfer_response {
> > > -     __u8                            data[0];        /* inbound data */
> > > -} __packed;
> >
> > As I said before, you can't just delete structures that are exported to
> > userspace without breaking things.  Why is this change acceptable to do
> > that?
> >
> > And how was any of this tested?
> >
> > greg k-h
> 
> Could you please give some examples that will be broken by this change?

Have you searched all userspace tools to verify that they do not use
this structure definition?  You are removing a user/kernel api here,
something that we do not do without researching that no existing user in
the world will not break.

> And I am not sure how this should be tested. It seems that it will not
> have any negative impact on functionality.

I would strongly recommend, that if you can not test this, that you not
make the change :)

good luck!

greg k-h

[PATCH v2] greybus: Avoid fake flexible array for response data

[Kees Cook]

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.work.862-kees@kernel.org/
---
 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 {
-- 
2.34.1

Re: [PATCH v2] greybus: Avoid fake flexible array for response data

[Alex Elder]

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.work.862-kees@kernel.org/
> ---
>   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 {

Re: [PATCH v2] greybus: Avoid fake flexible array for response data

[Kees Cook]

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

-- 
Kees Cook