linux-hardening - 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] [day] [month] [year] [list]

Message-ID: <412741af88854676b3070a0e378584dc52dd6905.camel@sipsolutions.net>
Date:   Tue, 21 Mar 2023 09:13:40 +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

On Mon, 2023-03-20 at 12:52 -0700, Kees Cook wrote:
> 
> What sort of patch would you like here? How should the other cases in
> the switch statement behave?
> 
> Are these the correct bounds?
> 
> 	WLAN_CIPHER_SUITE_CCMP:   keylen <= 16
> 	WLAN_CIPHER_SUITE_TKIP:   keylen <= 16
> 	WLAN_CIPHER_SUITE_WEP104: keylen <= 13
> 	WLAN_CIPHER_SUITE_WEP40:  keylen <= 13

40 bits is only 5 bytes :-)

> and should it silently ignore larger values in each case?
> 

For the cases other than TKIP, no changes should be necessary - in those
cases, the key will always be == the value from the (corrected) table
above.

For TKIP, the keylen will be 32, but comprised of the actual encryption
key and then MIC keys, so only 16 bytes should be relevant.

I don't really care how you handle this.

We can be bug-compatible with the old code, then I'd say from your patch
only the changes in the TKIP case are needed.

Or we can just limit the copy to only 16 bytes, but that would need some
validation that it still works. From memory, I'd say it will still work,
and I'd even say the whole memcpy() might not be needed in the TKIP case
at all since the iwldvm firmware deals with phase 1 keys (P1K) to derive
phase 2 keys, not the original encryption key.

So simpler is probably to just take the TKIP hunk from your patch, even
knowing that the memcpy() there is almost certainly incorrect now.

johannes