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 for Android: free password hash cracker in your pocket

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

Message-ID: <20220412072458.GC3293@kadam>
Date:   Tue, 12 Apr 2022 10:24:58 +0300
From:   Dan Carpenter <dan.carpenter@...cle.com>
To:     Jaehee Park <jhpark1013@...il.com>
Cc:     Johan Hovold <johan@...nel.org>, Alex Elder <elder@...nel.org>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.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

No, this patch is not right.

On Mon, Apr 11, 2022 at 05:14:11PM -0400, Jaehee Park wrote:
> 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[];
>  };
>  
>  struct gb_usb_device {
> @@ -102,16 +103,14 @@ static int hub_control(struct usb_hcd *hcd, u16 typeReq, u16 wValue, u16 wIndex,
>  	struct gb_operation *operation;
>  	struct gb_usb_hub_control_request *request;
>  	struct gb_usb_hub_control_response *response;
> -	size_t response_size;
>  	int ret;
>  
>  	/* FIXME: handle unspecified lengths */
> -	response_size = sizeof(*response) + wLength;

You're mixing up the value of wLength with the size of wLength (2).

>  
>  	operation = gb_operation_create(dev->connection,
>  					GB_USB_TYPE_HUB_CONTROL,
>  					sizeof(*request),
> -					response_size,
> +					sizeof(*response),

In the original code response_size was equal to wLength.  But now you're
passing 2.

So, I mean the no brainer approach would be to just say:

-					response_size,
+					wLength,

And delete the gb_usb_hub_control_response completely along with the
reference to it.

But better to do a brainer approach and investigate how that response
buffer is used.  It's probably all fine.  So probably the no brainer
approach is the correct approach.  It makes the code look nicer, it
doesn't break anything and we will merge it.  But better to at least
look carefully at it first.

regards,
dan carpenter