netdev - Re: Variables being modified but not used in net/wireless/lib80211_crypt

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: <379c3360c8e675532e48d7f2e6cc99f4f98c0fbe.camel@sipsolutions.net>
Date:   Tue, 22 Nov 2022 22:35:53 +0100
From:   Johannes Berg <johannes@...solutions.net>
To:     "Colin King (gmail)" <colin.i.king@...il.com>,
        "David S. Miller" <davem@...emloft.net>,
        Eric Dumazet <edumazet@...gle.com>,
        Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>
Cc:     "linux-wireless@...r.kernel.org" <linux-wireless@...r.kernel.org>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: Variables being modified but not used in
 net/wireless/lib80211_crypt_tkip.c

Hi,

Sorry it took me so long to get to this ...

This is ancient code, FWIW, and likely almost never used :-)

> I was reviewing some clang scan build static analysis results and found 
> an interesting warning:
> 
> Source: net/wireless/lib80211_crypt_tkip.c
> 
> net/wireless/lib80211_crypt_tkip.c:667:7: warning: variable 'iv32' set 
> but not used [-Wunused-but-set-variable]
>                  u32 iv32 = tkey->tx_iv32;
> 
> The variables iv32 and iv16 are being decremented, but are not 
> referenced after that. The seq[] array is being updated with the 
> pre-decremented values. Is that correct?
> 
>          if (seq) {
>                  /* Return the sequence number of the last transmitted 
> frame. */
>                  u16 iv16 = tkey->tx_iv16;
>                  u32 iv32 = tkey->tx_iv32;
>                  if (iv16 == 0)
>                          iv32--;
>                  iv16--;
>                  seq[0] = tkey->tx_iv16;
>                  seq[1] = tkey->tx_iv16 >> 8;
>                  seq[2] = tkey->tx_iv32;
>                  seq[3] = tkey->tx_iv32 >> 8;
>                  seq[4] = tkey->tx_iv32 >> 16;
>                  seq[5] = tkey->tx_iv32 >> 24;
>          }
> 

By the comment, that's not correct, and should use iv16/iv32 in the
seq[] assignments, since lib80211_tkip_hdr() increments tx_iv16/32
*after* setting it in the frame.

That said only some really ancient ioctls can even reach this
(prism2_ioctl_giwencodeext, prism2_ioctl_get_encryption) and then it
will be used by hostapd only in AP mode (also likely less used than
client mode) to send the seqno of the GTK on GTK rekeying to the client,
and then the client will (hopefully) use it to drop replays ...

So looks like worst case the client would drop a single frame because of
this, unless of course that frame was anyway already transmitted while
the whole rekeying was in progress...

johannes