Re: [PATCH 2/3] Replace WPA_ALG_PMK with KEY_FLAG_PMK

Am 01.03.20 um 18:37 schrieb Jouni Malinen:
> On Thu, Feb 27, 2020 at 10:52:47PM +0100, Alexander Wetzel wrote:
>> Drop the no longer needed internal alg WPA_ALG_PMK and use KEY_FLAG_PMK
>> as replacement.
> 
>> This is of course fully optional. With the key_flag API now merged WPA_ALG_PMK
>> just looks redundant and the few users of it are easily converted.
> 
> This is probably fine as a general direction, but I'm a bit concerned
> about the use of this type of comparison of key_flag which is defined as
> a bitmap:
> 
>> @@ -3093,7 +3092,7 @@ static int wpa_driver_nl80211_set_key(struct i802_bss *bss,
>> -	if (alg == WPA_ALG_PMK &&
>> +	if (key_flag == KEY_FLAG_PMK &&

I decided to block anything the driver is not coded for to prevent 
accidents like the existing one where we install pairwise keys also as 
unicast default keys. (Which never get used, though)
A direct compare is just nicely making sure no other flags can be set at 
the price of needing a code update if there ever is another flag which 
can be combined with KEY_FLAG_PMK.

But I see what you mean and will see how it looks with flag logic.
Guess I try a mask to only check known invalid bits instead blocking all 
undefined flags as replacement.

> 
> Sure, we do not really use KEY_FLAG_PMK with any other bit set, but
> still, comparing key_flag to any fixed value looks a bit strange and
> potentially easy to break should we ever add some more flag bits that
> could be used even with KEY_FLAG_PMK.
> 

Correct. But then any new usage of key_flags would need updates to the 
drivers anyhow. It would just force you to update first the driver and 
then the remaining code, which can be inconvenient.

>>   	    (drv->capa.flags & WPA_DRIVER_FLAGS_4WAY_HANDSHAKE_8021X))
>>   		return nl80211_set_pmk(drv, key, key_len, addr);
>>   	if (key_flag & KEY_FLAG_PMK)
> 
> And here is the way of comparing it against the specific bit for PMK in
> a manner that survives addition of new bits to the key_flag bitmap.
> 

This is in fact complementing the first "key_flag == KEY_FLAG_PMK" check 
from above. After this check we know KEY_FLAG_PMK is never set and don't 
have to handle strange combinations using it.

Alexander