Re: [PATCH v2 0/5] wifi: add opt-in FIPS exception for iwlwifi

[Johannes Berg]

Hi Jose,

I'm super confused by this, it looks like mostly random code changes to
not really do anything useful any more ... Nor do the code changes seem
to actually be doing what you describe.

> Commits 5241526dede9 ("wifi: mac80211: don't send keys to driver when
> fips_enabled") and 0636800c8ee1 ("wifi: iwlwifi: disable certain features
> for fips_enabled") disabled WiFi functionality under FIPS mode because
> Intel firmware autonomously sends some management frames without
> FIPS-validated integrity protection.
> 
> While this is technically correct, it leaves FIPS-required environments
> with no WiFi connectivity at all, since WPA3-SAE mandates MFP and without
> MFP_CAPABLE the client cannot even associate.

Arguably, that's what FIPS wanted. It mandates that you cannot give keys
to the device since it's not certified, and therefore it cannot do the
necessary functionality for these connections.

I can understand why you don't like it, and I guess we can try to find
ways around it like what you fundamentally seem to try to be doing here,
i.e. formulate an exception to the policies.

But I think you should actually formulate the exception that you *want*
first and then implement it, not randomly poke holes into the code and
call it done.

> Patch 1: Adds the fips_exception infrastructure (boot parameter,
>          read-only sysctl, fips_allows() helper)

Not my domain, but I'd argue that design documentation I ask for above
should somehow make it into the documentation for this parameter so taht
administrators can actually make an informed decision.

> Patch 2: Gates the mac80211 key blocking with fips_allows() so keys
>          can reach the firmware for data traffic

This patch is mostly wrong. If the exception is called "MFP" then
there's no need to pass all keys, and in fact the way you implemented
it, I don't even see how it doesn't end up with HW crypto after all.

> Patch 3: Gates the iwlwifi feature disabling with fips_allows(),
>          restoring MFP, Beacon Protection, EHT, 6GHz, A-MSDU sizes
>          and MLO support

This is also partially wrong - you didn't really understand this and
just undid everything?

> Patch 4: Best effort to force software encryption for unicast
>          management frames (CCMP/GCMP). It sets
>          IEEE80211_KEY_FLAG_SW_MGMT_TX on pairwise CCMP/GCMP keys when the
>          exception is active, forcing mac80211 to encrypt unicast robust
>          management frames (SA Query, deauth, disassoc) in software using
>          FIPS-approved CCMP/GCMP, the same mechanism used by ath9k, ath5k,
>          carl9170, mt76x02, rtw88, rtw89, rtlwifi, ... and other drivers.

This makes no sense at all.

> Patch 5: Reduces firmware decryption error message to debug level in
>          FIPS mode. Same as v1 patch 2/2, accepted upstream but not
>          yet landed.

That seems reasonable.

I'm not going to reply to the individual patches, but I observe that you
didn't understand IGTK functionality, WoWLAN, A-MSDUs, or maybe
encryption offload in general. From what I can tell, your patches are
mostly equivalent to turning FIPS off for wifi.

I don't know where to go from here. I don't think I'm going to teach you
all the necessary things here in the context of an upstream review, or
redo the patches correctly myself. Maybe you can approach Intel over the
distro channel Redhat has and ask them to help. Which will almost
certainly end up falling back to me, but at least then we can support it
and it's accounted for.

johannes