Variables being modified but not used in net/wireless/lib80211_crypt_tkip.c

Variables being modified but not used in net/wireless/lib80211_crypt_tkip.c

[Colin King (gmail)]

Hi,

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;
         }

         return TKIP_KEY_LEN;

Colin

Re: Variables being modified but not used in net/wireless/lib80211_crypt_tkip.c

[Johannes Berg]

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