linux-kernel - Re: [PATCH] usb: gadget: dummy: fix nonsensical comparisons

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:   Tue, 5 Sep 2017 10:30:38 -0400 (EDT)
From:   Alan Stern <stern@...land.harvard.edu>
To:     Arnd Bergmann <arnd@...db.de>
cc:     Felipe Balbi <balbi@...nel.org>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Tatyana Brokhman <tlinder@...eaurora.org>,
        Felipe Balbi <balbi@...com>, <linux-usb@...r.kernel.org>,
        <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] usb: gadget: dummy: fix nonsensical comparisons

On Tue, 5 Sep 2017, Arnd Bergmann wrote:

> gcc-8 points out two comparisons that are clearly bogus
> and almost certainly not what the author intended to write:
> 
> drivers/usb/gadget/udc/dummy_hcd.c: In function 'set_link_state_by_speed':
> drivers/usb/gadget/udc/dummy_hcd.c:379:31: error: bitwise comparison always evaluates to false [-Werror=tautological-compare]
>          USB_PORT_STAT_ENABLE) == 1 &&
>                                ^~
> drivers/usb/gadget/udc/dummy_hcd.c:381:25: error: bitwise comparison always evaluates to false [-Werror=tautological-compare]
>       USB_SS_PORT_LS_U0) == 1 &&
>                          ^~
> 
> I looked at the code for a bit and came up with a change that makes
> it look like what the author probably meant here. This makes it
> look reasonable to me and to gcc, shutting up the warning.
> 
> It does of course change behavior as the two conditions are actually
> evaluated rather than being hardcoded to false, and I have made no
> attempt at verifying that the changed logic makes sense in the context
> of a USB HCD, so that part needs to be reviewed carefully.
> 
> Fixes: 1cd8fd2887e1 ("usb: gadget: dummy_hcd: add SuperSpeed support")
> Cc: Tatyana Brokhman <tlinder@...eaurora.org>
> Cc: Felipe Balbi <balbi@...com>
> Signed-off-by: Arnd Bergmann <arnd@...db.de>
> ---
>  drivers/usb/gadget/udc/dummy_hcd.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/usb/gadget/udc/dummy_hcd.c b/drivers/usb/gadget/udc/dummy_hcd.c
> index a030d7923d7d..54e8e37d2bc8 100644
> --- a/drivers/usb/gadget/udc/dummy_hcd.c
> +++ b/drivers/usb/gadget/udc/dummy_hcd.c
> @@ -376,10 +376,10 @@ static void set_link_state_by_speed(struct dummy_hcd *dum_hcd)
>  				dum_hcd->port_status |=
>  					(USB_PORT_STAT_C_CONNECTION << 16);
>  			if ((dum_hcd->port_status &
> -			     USB_PORT_STAT_ENABLE) == 1 &&
> -				(dum_hcd->port_status &
> -				 USB_SS_PORT_LS_U0) == 1 &&
> -				dum_hcd->rh_state != DUMMY_RH_SUSPENDED)
> +			     USB_PORT_STAT_ENABLE) == USB_PORT_STAT_ENABLE &&

This test can simply become (dum_hcd->port_status & USB_PORT_STAT_ENABLE),
since the latter is a single bit.  If you don't mind making its form 
different from the forms of the other two tests.

Alan Stern

> +			    (dum_hcd->port_status &
> +			     USB_PORT_STAT_LINK_STATE) == USB_SS_PORT_LS_U0 &&
> +			    dum_hcd->rh_state != DUMMY_RH_SUSPENDED)
>  				dum_hcd->active = 1;
>  		}
>  	} else {
>