Re: [PATCH 4/4] nl80211: Add MLME primitives to support external SME

[Johannes Berg <johannes@sipsolutions.net>]

[-- Attachment #1: Type: text/plain, Size: 2224 bytes --]

On Thu, 2009-03-19 at 13:39 +0200, Jouni Malinen wrote:

> +	if (info->attrs[NL80211_ATTR_WIPHY_FREQ]) {
> +		req.chan = ieee80211_get_channel(
> +			wiphy,
> +			nla_get_u32(info->attrs[NL80211_ATTR_WIPHY_FREQ]));
> +		if (!req.chan) {
> +			err = -EINVAL;
> +			goto out;
> +		}
> +	}

Validate the channel is permitted?

> +	if (info->attrs[NL80211_ATTR_AUTH_TYPE]) {
> +		req.auth_type =
> +			nla_get_u32(info->attrs[NL80211_ATTR_AUTH_TYPE]);
> +	}

We should validate the auth type is one of the four valid ones, I think.

> +	if (nla_len(info->attrs[NL80211_ATTR_SSID]) > IEEE80211_MAX_SSID_LEN) {
> +		err = -EINVAL;
> +		goto out;
> +	}

That seems unnecessary since your netlink policy restricts it already.

> +	if (info->attrs[NL80211_ATTR_REASON_CODE])
> +		req.reason_code =
> +			nla_get_u16(info->attrs[NL80211_ATTR_REASON_CODE]);

Should probably require non-zero? Same in disassoc, I guess.

Since disassoc and deauth are really almost the same, maybe use one
function and multiplex like nl80211_addset_beacon?


> +static int ieee80211_auth(struct wiphy *wiphy, struct net_device *dev,
> +			  struct cfg80211_auth_request *req)
> +{
> +	struct ieee80211_sub_if_data *sdata;
> +
> +	if (!netif_running(dev))
> +		return -ENETDOWN;

Should probably be in cfg80211 directly, I think, to enforce this across
drivers.

> +	sdata = IEEE80211_DEV_TO_SUB_IF(dev);
> +
> +	if (sdata->vif.type != NL80211_IFTYPE_STATION)
> +		return -EOPNOTSUPP;

That too.

> +	switch (req->auth_type) {
> +	case NL80211_AUTHTYPE_OPEN_SYSTEM:
> +		sdata->u.mgd.auth_algs = IEEE80211_AUTH_ALG_OPEN;
> +		break;
> +	case NL80211_AUTHTYPE_SHARED_KEY:
> +		sdata->u.mgd.auth_algs = IEEE80211_AUTH_ALG_SHARED_KEY;
> +		break;
> +	case NL80211_AUTHTYPE_FT:
> +		sdata->u.mgd.auth_algs = IEEE80211_AUTH_ALG_FT;
> +		break;
> +	case NL80211_AUTHTYPE_NETWORK_EAP:
> +		sdata->u.mgd.auth_algs = IEEE80211_AUTH_ALG_LEAP;
> +		break;
> +	default:
> +		return -EOPNOTSUPP;

and I already said the "default" part here should be too (though it
should stick around here if we ever add new mechanisms)

Similarly in assoc, of course.

otherwise looks good.

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]