netdev - Re: [PATCH] wifi: iwlwifi: dvm: Add struct_group for struct iwl

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: <0ec5fe8b6945ee545b335ef2f3bee75b0af458d0.camel@sipsolutions.net>
Date:   Mon, 20 Mar 2023 19:34:59 +0100
From:   Johannes Berg <johannes@...solutions.net>
To:     Kees Cook <keescook@...omium.org>
Cc:     Gregory Greenman <gregory.greenman@...el.com>,
        Kalle Valo <kvalo@...nel.org>,
        "David S. Miller" <davem@...emloft.net>,
        Eric Dumazet <edumazet@...gle.com>,
        Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>,
        Benjamin Berg <benjamin.berg@...el.com>,
        Sriram R <quic_srirrama@...cinc.com>,
        lukasz.wojnilowicz@...il.com, linux-wireless@...r.kernel.org,
        netdev@...r.kernel.org, linux-kernel@...r.kernel.org,
        linux-hardening@...r.kernel.org
Subject: Re: [PATCH] wifi: iwlwifi: dvm: Add struct_group for struct
 iwl_keyinfo keys

> > 
> > >  	case WLAN_CIPHER_SUITE_TKIP:
> > >  		key_flags |= STA_KEY_FLG_TKIP;
> > >  		sta_cmd.key.tkip_rx_tsc_byte2 = tkip_iv32;
> > >  		for (i = 0; i < 5; i++)
> > >  			sta_cmd.key.tkip_rx_ttak[i] = cpu_to_le16(tkip_p1k[i]);
> > > -		memcpy(sta_cmd.key.key, keyconf->key, keyconf->keylen);
> > > +		memcpy(&sta_cmd.key.keys, keyconf->key, keyconf->keylen);
> > 
> > And that's actually a bug, we should've copied only 16 bytes, I guess.
> > DVM didn't support MIC offload anyway (at least the way Linux uses the
> > firmware, though I thought it doesn't at all), so we don't need the MIC
> > RX/TX keys in there, but anyway the sequence counter values are not part
> > of the key material on the host.
> > 
> > I don't think I have a machine now to test this with (nor a TKIP AP, of
> > course, but that could be changed) - but I suspect that since we
> > actually calculate the TTAK above, we might not even need this memcpy()
> > at all?
> 
> It's the latter that is triggered in the real world, though. See the
> referenced URL and also now on bugzilla:
> https://bugzilla.kernel.org/show_bug.cgi?id=217214
> i.e.: drivers/net/wireless/intel/iwlwifi/dvm/sta.c:1103
> 
> So keyconf->keylen is coming in as 32. If this is a bug, I'm not sure
> where/how to fix it.

Yes, I know it's coming in as such - I believe it should be copying 16
bytes instead of the full keylen. TKIP keys are comprised of 16 bytes
encryption/decryption key and 8 bytes TX/RX MIC keys for a total of 32,
but since the device doesn't do MIC calculations, it only needs the
first 16 bytes here (if even that, since we also give it the P1K which
is derived from the TK...? maybe not even that)

But I guess we should test it ... not sure I still have a machine that
takes these NICs (I do have NICs).

johannes