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

Kees Cook
2025-05-27T03:36:05Z

On Mon, May 26, 2025 at 02:37:13PM +0300, Dan Carpenter wrote:
> On Mon, May 26, 2025 at 07:06:54PM +0800, clingfei wrote:
> > As https://lore.kernel.org/all/20240304211940.it.083-kees@kernel.org/
>
> I don't want to have to read a link to understand the commit message.
>
> Does this change really affect anything in terms of "enforce the 0-sized
> destinations" rule? I don't think this is a destination. I think Kees
> enabled the checking he wanted. You should probably CC him since we're
> refering to his email.

FWIW, the above patch became commit 7ba59ac7da2a ("greybus: Avoid fake
flexible array for response data").

> > pointed out, to enforce the 0-sized destinations, the remaining 0-sized
> > destinations need to be handled. Thus the struct
> > gb_control_get_manifest_response and struct gb_i2c_transfer_response
> > are removed.
>
> Here is a better commit message;
>
> "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 char pointer instead."
>
> I would have ignored it, probably but actually the
> gb_control_get_manifest_response struct is not used so put that in a
> separate commit. That's a simpler commit to review.
>
> "The gb_control_get_manifest_response struct is not used. Delete it."

The patch content itself looks mechanically correct. I think Dan's style
suggestions would be good to see. Can you send a v2?

-Kees

>
> >
> > Signed-off-by: clingfei <clf700383@gmail.com>
> > ---
> > drivers/staging/greybus/i2c.c | 9 ++++-----
> > include/linux/greybus/greybus_protocols.h | 9 ---------
> > 2 files changed, 4 insertions(+), 14 deletions(-)
> >
> > diff --git a/drivers/staging/greybus/i2c.c b/drivers/staging/greybus/i2c.c
> > index 14f1ff6d448c..2857c2834206 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,7 +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;
> > + u8 *response;
> >
> > response = operation->response->payload;
> > gb_i2c_decode_response(msgs, msg_count, response);
>
> I like when parameters are called the same thing on both sides. The
> name "response" adds no value. Instead get rid of that variable and
> pass operation->response->payload directly.
>
> gb_i2c_decode_response(msgs, msg_count,
> operation->response->payload);
>
> regards,
> dan carpenter
>

--
Kees Cook