Re: [RFC PATCH v3 02/12] nl80211/cfg80211: Extended Key ID support

[Johannes Berg <johannes@sipsolutions.net>]

On Sun, 2019-02-10 at 22:06 +0100, Alexander Wetzel wrote:
> +/**
> + * enum nl80211_key_install_mode - Key install mode
> + *
> + * @NL80211_KEY_RX_TX: Key must be installed for Rx and Tx
> + * @NL80211_KEY_RX_ONLY: Allowed in combination with @NL80211_CMD_NEW_KEY:
> + *	Unicast key has to be installed for Rx only.
> + * @NL80211_KEY_SWITCH_TX: Allowed in combination with @NL80211_CMD_SET_KEY:
> + *	Switch Tx to a Rx only, referenced by sta mac and idx.

Don't you mean the other way around? Or, well, what you say is true for
the *other* keys?

>   *	by the %NL80211_SCAN_FLAG_MIN_PREQ_CONTENT flag.
>   * @NL80211_EXT_FEATURE_ENABLE_FTM_RESPONDER: Driver supports enabling fine
>   *	timing measurement responder role.
> - *

no need to remove that :)

> -	/* only support setting default key */
> -	if (!key.def && !key.defmgmt)
> +	/* Only support setting default key and
> +	 * Extended Key ID action @NL80211_KEY_SWITCH_TX.
> +	 */

you can remove the @, it's not a kernel-doc formatted comment

> -	}
> +	} else if (key.p.install_mode == NL80211_KEY_SWITCH_TX &&
> +		   wiphy_ext_feature_isset(&rdev->wiphy,
> +					   NL80211_EXT_FEATURE_EXT_KEY_ID)) {
> +		u8 *mac_addr = NULL;
>  
> +		if (info->attrs[NL80211_ATTR_MAC])
> +			mac_addr = nla_data(info->attrs[NL80211_ATTR_MAC]);
> +
> +		if (!mac_addr || key.idx < 0 || key.idx > 1) {
> +			err = -EINVAL;
> +			goto out;
> +		}

Really only 0 and 1 are allowed? Not 0-3?

> +++ b/net/wireless/util.c
> @@ -236,14 +236,22 @@ int cfg80211_validate_key_settings(struct cfg80211_registered_device *rdev,
>  	case WLAN_CIPHER_SUITE_CCMP_256:
>  	case WLAN_CIPHER_SUITE_GCMP:
>  	case WLAN_CIPHER_SUITE_GCMP_256:
> -		/* Disallow pairwise keys with non-zero index unless it's WEP
> -		 * or a vendor specific cipher (because current deployments use
> -		 * pairwise WEP keys with non-zero indices and for vendor
> -		 * specific ciphers this should be validated in the driver or
> -		 * hardware level - but 802.11i clearly specifies to use zero)
> +		/* IEEE802.11-2016 allows only 0 and - when using Extended Key
> +		 * ID - 1 as index for pairwise keys.
> +		 * @NL80211_KEY_RX_ONLY is only allowed for pairwise keys when
> +		 * the driver supports Extended Key ID.
> +		 * @NL80211_KEY_SWITCH_TX must not be set when validating a key.
>  		 */
> -		if (pairwise && key_idx)
> +		if (params->install_mode == NL80211_KEY_RX_ONLY) {
> +			if (!wiphy_ext_feature_isset(&rdev->wiphy,
> +						     NL80211_EXT_FEATURE_EXT_KEY_ID))
> +				return -EINVAL;
> +			else if (!pairwise || key_idx < 0 || key_idx > 1)
> +				return -EINVAL;

same question here

johannes