linux-kernel - Re: [PATCH] staging: rtl8188eu: Simplify memcmp logical checks

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]
Message-ID: <20180925191504.GA4454@flashbox>
Date:   Tue, 25 Sep 2018 12:15:04 -0700
From:   Nathan Chancellor <natechancellor@...il.com>
To:     Greg Kroah-Hartman <gregkh@...uxfoundation.org>
Cc:     devel@...verdev.osuosl.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] staging: rtl8188eu: Simplify memcmp logical checks

On Tue, Sep 25, 2018 at 09:07:11PM +0200, Greg Kroah-Hartman wrote:
> On Thu, Sep 20, 2018 at 05:22:21PM -0700, Nathan Chancellor wrote:
> > Clang generates a warning when it sees a logical not followed by a
> > conditional operator like ==, >, or < because it thinks that the logical
> > not should be applied to the whole statement:
> > 
> > drivers/staging/rtl8188eu/core/rtw_ieee80211.c:293:8: warning: logical
> > not is only applied to the left hand side of this comparison
> > [-Wlogical-not-parentheses]
> > 
> > It assumes the author might have made a mistake in their logic:
> > 
> > if (!a == b) -> if (!(a == b))
> > 
> > Sometimes that is the case; other times, it's just a super convoluted
> > way of saying 'if (a)' when b = 0:
> > 
> > if (!1 == 0) -> if (0 == 0) -> if (true)
> > 
> > Alternatively:
> > 
> > if (!1 == 0) -> if (!!1) -> if (1)
> > 
> > Simplify these comparisons so that Clang doesn't complain.
> > 
> > Link: https://github.com/ClangBuiltLinux/linux/issues/161
> > Signed-off-by: Nathan Chancellor <natechancellor@...il.com>
> > ---
> >  drivers/staging/rtl8188eu/core/rtw_ieee80211.c | 2 +-
> >  drivers/staging/rtl8188eu/core/rtw_mlme.c      | 2 +-
> >  drivers/staging/rtl8188eu/core/rtw_recv.c      | 4 ++--
> >  drivers/staging/rtl8188eu/core/rtw_wlan_util.c | 2 +-
> >  4 files changed, 5 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/staging/rtl8188eu/core/rtw_ieee80211.c b/drivers/staging/rtl8188eu/core/rtw_ieee80211.c
> > index 9e5c7e62d26f..20f34d25c369 100644
> > --- a/drivers/staging/rtl8188eu/core/rtw_ieee80211.c
> > +++ b/drivers/staging/rtl8188eu/core/rtw_ieee80211.c
> > @@ -284,7 +284,7 @@ unsigned char *rtw_get_wpa_ie(unsigned char *pie, uint *wpa_ie_len, int limit)
> >  
> >  		if (pbuf) {
> >  			/* check if oui matches... */
> > -			if (!memcmp((pbuf + 2), wpa_oui_type, sizeof(wpa_oui_type)) == false)
> > +			if (memcmp((pbuf + 2), wpa_oui_type, sizeof(wpa_oui_type)))
> >  				goto check_next_ie;
> >  
> >  			/* check version... */
> > diff --git a/drivers/staging/rtl8188eu/core/rtw_mlme.c b/drivers/staging/rtl8188eu/core/rtw_mlme.c
> > index ef8a7dc4bd34..43d6513484c5 100644
> > --- a/drivers/staging/rtl8188eu/core/rtw_mlme.c
> > +++ b/drivers/staging/rtl8188eu/core/rtw_mlme.c
> > @@ -1437,7 +1437,7 @@ static int rtw_check_join_candidate(struct mlme_priv *pmlmepriv
> >  	/* check ssid, if needed */
> >  	if (pmlmepriv->assoc_ssid.SsidLength) {
> >  		if (competitor->network.Ssid.SsidLength != pmlmepriv->assoc_ssid.SsidLength ||
> > -		    !memcmp(competitor->network.Ssid.Ssid, pmlmepriv->assoc_ssid.Ssid, pmlmepriv->assoc_ssid.SsidLength) == false)
> > +		    memcmp(competitor->network.Ssid.Ssid, pmlmepriv->assoc_ssid.Ssid, pmlmepriv->assoc_ssid.SsidLength))
> >  			goto exit;
> >  	}
> >  
> > diff --git a/drivers/staging/rtl8188eu/core/rtw_recv.c b/drivers/staging/rtl8188eu/core/rtw_recv.c
> > index ab9638d618a9..f3eb63f8cf0b 100644
> > --- a/drivers/staging/rtl8188eu/core/rtw_recv.c
> > +++ b/drivers/staging/rtl8188eu/core/rtw_recv.c
> > @@ -1283,8 +1283,8 @@ static int wlanhdr_to_ethhdr(struct recv_frame *precvframe)
> >  	psnap_type = ptr+pattrib->hdrlen + pattrib->iv_len+SNAP_SIZE;
> >  	/* convert hdr + possible LLC headers into Ethernet header */
> >  	if ((!memcmp(psnap, rtw_rfc1042_header, SNAP_SIZE) &&
> > -	     (!memcmp(psnap_type, SNAP_ETH_TYPE_IPX, 2) == false) &&
> > -	     (!memcmp(psnap_type, SNAP_ETH_TYPE_APPLETALK_AARP, 2) == false)) ||
> > +	     memcmp(psnap_type, SNAP_ETH_TYPE_IPX, 2) &&
> > +	     memcmp(psnap_type, SNAP_ETH_TYPE_APPLETALK_AARP, 2)) ||
> >  	     !memcmp(psnap, rtw_bridge_tunnel_header, SNAP_SIZE)) {
> >  		/* remove RFC1042 or Bridge-Tunnel encapsulation and replace EtherType */
> >  		bsnaphdr = true;
> > diff --git a/drivers/staging/rtl8188eu/core/rtw_wlan_util.c b/drivers/staging/rtl8188eu/core/rtw_wlan_util.c
> > index fb496ab5a862..51cf78150168 100644
> > --- a/drivers/staging/rtl8188eu/core/rtw_wlan_util.c
> > +++ b/drivers/staging/rtl8188eu/core/rtw_wlan_util.c
> > @@ -856,7 +856,7 @@ int rtw_check_bcn_info(struct adapter  *Adapter, u8 *pframe, u32 packet_len)
> >  		return _FAIL;
> >  	}
> >  
> > -	if (!memcmp(cur_network->network.MacAddress, pbssid, 6) == false) {
> > +	if (memcmp(cur_network->network.MacAddress, pbssid, 6) {
> >  		DBG_88E("Oops: rtw_check_network_encrypt linked but recv other bssid bcn\n%pM %pM\n",
> >  			(pbssid), (cur_network->network.MacAddress));
> >  		return true;
> 
> Always test-build your patches before sending them off so you do not get
> a grumpy maintainer upset at you for breaking their build system...
> 
> Hint, it's the last chunk here...
> 
> greg k-h

Gah sorry, it's always the times that I don't do that that something bad
happens :(

v2 sent.

Nathan