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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CADPKJ-64_fod0ObZsg_prtB4u+ZA6shZ6AnXqn4vxK1NGxHgkQ@mail.gmail.com>
Date: Tue, 27 May 2025 13:50:42 +0800
From: clingfei <clf700383@...il.com>
To: Greg KH <gregkh@...uxfoundation.org>
Cc: elder@...nel.org, keescook@...omium.org, johan@...nel.org, 
	vireshk@...nel.org, greybus-dev@...ts.linaro.org, 
	linux-staging@...ts.linux.dev, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] greybus: Avoid fake flexible array for response data

Greg KH <gregkh@...uxfoundation.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@...il.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.