netdev - Re: Potential invalid ~ operator in net/mac80211/cfg.c

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: <1383c6f1-1317-daed-ecc7-e5cc3f309c41@canonical.com>
Date:   Fri, 5 Feb 2021 18:19:10 +0000
From:   Colin Ian King <colin.king@...onical.com>
To:     Johannes Berg <johannes@...solutions.net>
Cc:     "David S. Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        "linux-wireless@...r.kernel.org" <linux-wireless@...r.kernel.org>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>
Subject: Re: Potential invalid ~ operator in net/mac80211/cfg.c

On 05/02/2021 18:05, Johannes Berg wrote:
> Hi Colin,
> 
>> while working through a backlog of older static analysis reports from
>> Coverity
> 
> So ... yeah. Every time I look at Coverity (not frequently, I must
> admit) I see the same thing, and get confused.
> 
>> I found an interesting use of the ~ operator that looks
>> incorrect to me in function ieee80211_set_bitrate_mask():
>>
>>                 for (j = 0; j < IEEE80211_HT_MCS_MASK_LEN; j++) {
>>                         if (~sdata->rc_rateidx_mcs_mask[i][j]) {
>>                                 sdata->rc_has_mcs_mask[i] = true;
>>                                 break;
>>                         }
>>                 }
>>
>>                 for (j = 0; j < NL80211_VHT_NSS_MAX; j++) {
>>                         if (~sdata->rc_rateidx_vht_mcs_mask[i][j]) {
>>                                 sdata->rc_has_vht_mcs_mask[i] = true;
>>                                 break;
>>                         }
>>                 }
>>
>> For the ~ operator in both if stanzas, Coverity reports:
>>
>> Logical vs. bitwise operator (CONSTANT_EXPRESSION_RESULT)
>> logical_vs_bitwise:
>>
>> ~sdata->rc_rateidx_mcs_mask[i][j] is always 1/true regardless of the
>> values of its operand. This occurs as the logical operand of if.
>>     Did you intend to use ! rather than ~?
>>
>> I've checked the results of this and it does seem that ~ is incorrect
>> and always returns true for the if expression. So it probably should be
>> !, but I'm not sure if I'm missing something deeper here and wondering
>> why this has always worked.
> 
> But is it really always true?
> 
> I _think_ it was intended to check that it's not 0xffffffff or
> something?
> 
> https://lore.kernel.org/linux-wireless/516C0C7F.3000204@openwrt.org/
> 
> But maybe that isn't actually quite right due to integer promotion?
> OTOH, that's a u8, so it should do the ~ in u8 space, and then compare
> to 0 also?

rc_rateidx_vht_mcs_mask is a u64, so I think the expression could be
expressed as:

if ((uint16_t)~sdata->rc_rateidx_mcs_mask[i][j]) ..

this is only true if all the 16 bits in the mask are 0xffff

> 
> johannes
>