linux-kernel - Re: [PATCH] staging: greybus: replace zero-element array with flexible-array

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: <YlUGp+/BzMSm3oDC@kroah.com>
Date:   Tue, 12 Apr 2022 06:57:11 +0200
From:   Greg Kroah-Hartman <gregkh@...uxfoundation.org>
To:     Jaehee Park <jhpark1013@...il.com>
Cc:     Johan Hovold <johan@...nel.org>, Alex Elder <elder@...nel.org>,
        greybus-dev@...ts.linaro.org, linux-staging@...ts.linux.dev,
        linux-kernel@...r.kernel.org, outreachy@...ts.linux.dev
Subject: Re: [PATCH] staging: greybus: replace zero-element array with
 flexible-array

On Tue, Apr 12, 2022 at 06:56:12AM +0200, Greg Kroah-Hartman wrote:
> On Mon, Apr 11, 2022 at 05:14:11PM -0400, Jaehee Park wrote:
> > Zero-length and one-element arrays are deprecated. Flexible-array
> > members should be used instead. Flexible-array members are
> > recommended because this is the way the kernel expects dynamically
> > sized trailing elements to be declared.
> > Refer to Documentation/process/deprecated.rst.
> > 
> > Change the zero-length array, buf, in the struct 
> > gb_usb_hub_control_response to a flexible array. And add wLength as a 
> > member of the struct so that the struct is not a zero-sized struct.
> > 
> > Issue found by flexible_array coccinelle script.
> > 
> > Signed-off-by: Jaehee Park <jhpark1013@...il.com>
> > ---
> > 
> > I have a question for the authors: 
> > I saw a fixme comment in the hub_control function in usb.c:
> > / FIXME: handle unspecified lengths /
> > 
> > I was wondering why this comment was left there?
> > 
> > In this patch, I'm using this struct:
> > 
> > struct gb_usb_hub_control_response {
> >     __le16 wLength;
> >     u8 buf[];
> > };
> > 
> > And instead of using response_size, I'm doing this:
> > 
> > struct gb_usb_hub_control_response *response;
> > And using sizeof(*response) as the input to gb_operation_create.
> > 
> > Would the flexible array address the handling of unspecified lengths 
> > issue (in the fixme comment)?
> 
> No, you can not change the format of the data on the bus without also
> changing the firmware in the device and usually the specification as
> well.
> 
> >  drivers/staging/greybus/usb.c | 7 +++----
> >  1 file changed, 3 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/staging/greybus/usb.c b/drivers/staging/greybus/usb.c
> > index 8e9d9d59a357..d0b2422401df 100644
> > --- a/drivers/staging/greybus/usb.c
> > +++ b/drivers/staging/greybus/usb.c
> > @@ -27,7 +27,8 @@ struct gb_usb_hub_control_request {
> >  };
> >  
> >  struct gb_usb_hub_control_response {
> > -	u8 buf[0];
> > +	__le16 wLength;
> > +	u8 buf[];
> 
> What is wrong with buf[0] here?
> 
> You can fix this in other ways if you really understand the difference
> between [0] and [] in C.  Please look at many of the other conversions
> if you wish to do this.

And I would not recommend this as an "outreachy introduction task"
unless you understand this.  There are much easier first patch tasks you
can accomplish instead.

good luck!

greg k-h