Received-SPF: pass (google.com: domain of linux-wireless+bounces-62-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Message-ID: <da6f058b85f91af5d4abd10bc56034f8d7811e86.camel@sipsolutions.net>
Subject: Re: [PATCH v3 1/1] wifi: nl80211: Extend del pmksa support for SAE
 and OWE security
From: Johannes Berg <johannes@sipsolutions.net>
To: Vinayak Yadawad <vinayak.yadawad@broadcom.com>
Cc: linux-wireless@vger.kernel.org, jithu.jance@broadcom.com
Date: Fri, 24 Nov 2023 19:58:33 +0100
In-Reply-To: <ff0778a86574b552769027496f12596e2e627931.1699530774.git.vinayak.yadawad@broadcom.com>
References: 
	<ff0778a86574b552769027496f12596e2e627931.1699530774.git.vinayak.yadawad@broadcom.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
User-Agent: Evolution 3.48.4 (3.48.4-1.fc38) 
Precedence: bulk
MIME-Version: 1.0

On Thu, 2023-11-09 at 18:00 +0530, Vinayak Yadawad wrote:
>=20
> +++ b/net/wireless/nl80211.c
> @@ -12183,24 +12183,37 @@ static int nl80211_setdel_pmksa(struct sk_buff =
*skb, struct genl_info *info)
> =20
>  	memset(&pmksa, 0, sizeof(struct cfg80211_pmksa));
> =20
> -	if (!info->attrs[NL80211_ATTR_PMKID])
> +	if ((info->genlhdr->cmd =3D=3D NL80211_CMD_SET_PMKSA) &&
> +	    (!info->attrs[NL80211_ATTR_PMKID]))
>  		return -EINVAL;

Maybe it'd be better to split set/del now? The code kind of looks
awkward now, don't you think?

Or split this part of the parsing depending on set or del?

> -	pmksa.pmkid =3D nla_data(info->attrs[NL80211_ATTR_PMKID]);
> +	if (info->attrs[NL80211_ATTR_PMKID])
> +		pmksa.pmkid =3D nla_data(info->attrs[NL80211_ATTR_PMKID]);
> =20
>  	if (info->attrs[NL80211_ATTR_MAC]) {
>  		pmksa.bssid =3D nla_data(info->attrs[NL80211_ATTR_MAC]);
> -	} else if (info->attrs[NL80211_ATTR_SSID] &&
> -		   info->attrs[NL80211_ATTR_FILS_CACHE_ID] &&
> -		   (info->genlhdr->cmd =3D=3D NL80211_CMD_DEL_PMKSA ||
> +	} else if (info->attrs[NL80211_ATTR_SSID]) {
> +		/* SSID based pmksa flush suppported only for FILS,
> +		 * OWE/SAE OFFLOAD cases
> +		 */
> +		if (info->attrs[NL80211_ATTR_FILS_CACHE_ID] &&
> +		    (info->genlhdr->cmd =3D=3D NL80211_CMD_DEL_PMKSA ||
>  		    info->attrs[NL80211_ATTR_PMK])) {
> +			pmksa.cache_id =3D
> +				nla_data(info->attrs[NL80211_ATTR_FILS_CACHE_ID]);
> +		} else if ((info->genlhdr->cmd =3D=3D NL80211_CMD_DEL_PMKSA) &&
> +		    (!wiphy_ext_feature_isset(
> +		    &rdev->wiphy, NL80211_EXT_FEATURE_SAE_OFFLOAD) &&
> +		    (!wiphy_ext_feature_isset(
> +		    &rdev->wiphy,NL80211_EXT_FEATURE_OWE_OFFLOAD)))){


The indentation here is also really awful ... I'd rather go over 80
columns than break like that. But you could just have local variables
for all the feature checks too.

And if you don't split set/del, I'd recommend a variable for that too,
set at the beginning, perhaps moving the "rdev_ops" thing up? But I'm
thinking it's probably nicer to split it. See how that looks like?

> +			return -EINVAL;
> +		}
>  		pmksa.ssid =3D nla_data(info->attrs[NL80211_ATTR_SSID]);
>  		pmksa.ssid_len =3D nla_len(info->attrs[NL80211_ATTR_SSID]);
> -		pmksa.cache_id =3D
> -			nla_data(info->attrs[NL80211_ATTR_FILS_CACHE_ID]);
>  	} else {
>  		return -EINVAL;
>  	}
> +
>  	if (info->attrs[NL80211_ATTR_PMK]) {

Then again, that isn't even relevant for DEL, is it? Should we even
parse it? Does anyone want to "delete only if it's exactly this PMK"?


Also seems like this should come with some nl80211.h updates though?

johannes