linux-kernel - Re: [PATCH] usb: gadget: net2280: Distribute switch variables for initialization

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] [day] [month] [year] [list]

Date:   Thu, 20 Feb 2020 10:34:36 -0500 (EST)
From:   Alan Stern <stern@...land.harvard.edu>
To:     Kees Cook <keescook@...omium.org>
cc:     Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Felipe Balbi <balbi@...nel.org>,
        Alexander Potapenko <glider@...gle.com>,
        <linux-usb@...r.kernel.org>, <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] usb: gadget: net2280: Distribute switch variables for
 initialization

On Wed, 19 Feb 2020, Kees Cook wrote:

> Variables declared in a switch statement before any case statements
> cannot be automatically initialized with compiler instrumentation (as
> they are not part of any execution flow). With GCC's proposed automatic
> stack variable initialization feature, this triggers a warning (and they
> don't get initialized). Clang's automatic stack variable initialization
> (via CONFIG_INIT_STACK_ALL=y) doesn't throw a warning, but it also
> doesn't initialize such variables[1]. Note that these warnings (or silent
> skipping) happen before the dead-store elimination optimization phase,
> so even when the automatic initializations are later elided in favor of
> direct initializations, the warnings remain.
> 
> To avoid these problems, move such variables into the "case" where
> they're used or lift them up into the main function body.
> 
> drivers/usb/gadget/udc/net2280.c: In function ‘handle_stat0_irqs_superspeed’:
> drivers/usb/gadget/udc/net2280.c:2871:22: warning: statement will never be executed [-Wswitch-unreachable]
>  2871 |   struct net2280_ep *e;
>       |                      ^
> 
> [1] https://bugs.llvm.org/show_bug.cgi?id=44916
> 
> Signed-off-by: Kees Cook <keescook@...omium.org>
> ---
>  drivers/usb/gadget/udc/net2280.c |   10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/usb/gadget/udc/net2280.c b/drivers/usb/gadget/udc/net2280.c
> index 1fd1b9186e46..cc5869363298 100644
> --- a/drivers/usb/gadget/udc/net2280.c
> +++ b/drivers/usb/gadget/udc/net2280.c
> @@ -2861,6 +2861,7 @@ static void ep_clear_seqnum(struct net2280_ep *ep)
>  static void handle_stat0_irqs_superspeed(struct net2280 *dev,
>  		struct net2280_ep *ep, struct usb_ctrlrequest r)
>  {
> +	struct net2280_ep *e;
>  	int tmp = 0;
>  
>  #define	w_value		le16_to_cpu(r.wValue)
> @@ -2868,14 +2869,13 @@ static void handle_stat0_irqs_superspeed(struct net2280 *dev,
>  #define	w_length	le16_to_cpu(r.wLength)
>  
>  	switch (r.bRequest) {
> -		struct net2280_ep *e;
> -		u16 status;
> -
>  	case USB_REQ_SET_CONFIGURATION:
>  		dev->addressed_state = !w_value;
>  		goto usb3_delegate;
>  
> -	case USB_REQ_GET_STATUS:
> +	case USB_REQ_GET_STATUS: {
> +		u16 status;
> +
>  		switch (r.bRequestType) {
>  		case (USB_DIR_IN | USB_TYPE_STANDARD | USB_RECIP_DEVICE):
>  			status = dev->wakeup_enable ? 0x02 : 0x00;
> @@ -2905,7 +2905,7 @@ static void handle_stat0_irqs_superspeed(struct net2280 *dev,
>  			goto usb3_delegate;
>  		}
>  		break;
> -
> +	}

This is an unusual use of nested blocks (i.e., a block immediately 
following a "case" label, and it throws the indentation off -- there 
will now be two nested closing braces at the same indentation level: 
this one and the one preceding do_stall3:.

Just define status at the function level, along with e, and don't 
worry about it.

Alan Stern

>  	case USB_REQ_CLEAR_FEATURE:
>  		switch (r.bRequestType) {
>  		case (USB_DIR_OUT | USB_TYPE_STANDARD | USB_RECIP_DEVICE):
> 
> 
>