Oops and other problems in cfg80211

Oops and other problems in cfg80211

[Quentin Armitage]

If cfg80211 is compiled with CONFIG_WIRELESS_OLD_REGULATORY defined, if
insmod is loaded with "insmod cfg80211 ieee80211_regdom=EU" then the
execution of insmod mac80211, executing insmod ath5k causes an oops.
This is caused by wiphy_update_regulatory being called when last_request
== NULL, and the call of reg_is_world_roaming (via reg_process_beacons)
causes the oops at last_request->initiator != REGDOM_SET_BY_COUNTRY_IE.

If reg_is_world_roaming is modified to check for last_request not being
NULL, e.g.
if (last_request && last_request->initiator != REGDOM_SET_BY_COUNTRY_IE
&&
then I also get an oops in reg_device_remove where last_request is
referenced and there is only a subsequent check for it being NULL.

Even if the above two are fixed, it will not associate with the access
point, and `iwlist wlan0 channels` produces a rather confused list. It
appears that, despite the comment, in regulatory_init it is necessary to
call regulatory_hint_core even for the pseudo country EU.

Patch1 below covers the points above, although I am not sure how useful
the BUG_ON (!last_request) in reg_is_world_roaming is.

Having modified the above, I then found that the regulatory domain was
not being updated from the Country IE sent by the access point. It
appears that the code following the comment "so we optimize an early
check ..." in regulatory_hint_11d is a little over optimized, since if
the following if statement is true, every path through that block ends
in "goto out;". Patch 2 below allows the Country IE to be processed
(note it also needs an extra check around a WARN_ON, and so the
preceeding comment to that needs considering).

Quentin Armitage

=========  Patch 1 ================================================
--- a/net/wireless/reg.c	2009-03-13 05:14:06.000000000 +0000
+++ b/net/wireless/reg.c	2009-03-16 16:57:03.000000000 +0000
@@ -1128,6 +1128,8 @@ static bool reg_is_world_roaming(struct 
 	if (is_world_regdom(cfg80211_regdomain->alpha2) ||
 	    (wiphy->regd && is_world_regdom(wiphy->regd->alpha2)))
 		return true;
+
+	BUG_ON (!last_request) ;
 	if (last_request->initiator != NL80211_REGDOM_SET_BY_COUNTRY_IE &&
 	    wiphy->custom_regulatory)
 		return true;
@@ -2105,7 +2107,8 @@ void reg_device_remove(struct wiphy *wip
 
 	assert_cfg80211_lock();
 
-	request_wiphy = wiphy_idx_to_wiphy(last_request->wiphy_idx);
+	if ( last_request )
+		request_wiphy = wiphy_idx_to_wiphy(last_request->wiphy_idx);
 
 	kfree(wiphy->regd);
 	if (!last_request || !request_wiphy)
@@ -2135,11 +2138,11 @@ int regulatory_init(void)
 	/*
 	 * The old code still requests for a new regdomain and if
 	 * you have CRDA you get it updated, otherwise you get
-	 * stuck with the static values. We ignore "EU" code as
-	 * that is not a valid ISO / IEC 3166 alpha2
+	 * stuck with the static values. We process "EU" code even though
+	 * that is not a valid ISO / IEC 3166 alpha2, since otherwise the
+	 * channels don't get set properly, and last_request is not set.
 	 */
-	if (ieee80211_regdom[0] != 'E' || ieee80211_regdom[1] != 'U')
-		err = regulatory_hint_core(ieee80211_regdom);
+	err = regulatory_hint_core(ieee80211_regdom);
 #else
 	cfg80211_regdomain = cfg80211_world_regdom;
 
===============================================================

============= Patch 2 =======================================
--- a/net/wireless/reg.c	2009-03-16 16:55:11.000000000 +0000
+++ b/net/wireless/reg.c	2009-03-16 16:57:57.000000000 +0000
@@ -1692,7 +1692,8 @@ void regulatory_hint_11d(struct wiphy *w
 			 * to another country, and then gets IEs from an
 			 * AP with different settings
 			 */
-			goto out;
+			if (likely(last_request->initiator ==
NL80211_REGDOM_SET_BY_COUNTRY_IE))
+				goto out;
 		} else {
 			/*
 			 * Ignore IEs coming in on two separate wiphys with
@@ -1721,8 +1722,11 @@ void regulatory_hint_11d(struct wiphy *w
 	 * If we hit this before we add this support we want to be informed of
 	 * it as it would indicate a mistake in the current design
 	 */
-	if (WARN_ON(reg_same_country_ie_hint(wiphy, checksum)))
-		goto free_rd_out;
+	if (likely(last_request->initiator ==
NL80211_REGDOM_SET_BY_COUNTRY_IE))
+	{
+		if (WARN_ON(reg_same_country_ie_hint(wiphy, checksum)))
+			goto free_rd_out;
+	}
 
 	request = kzalloc(sizeof(struct regulatory_request), GFP_KERNEL);
 	if (!request)
=============================================================

Re: Oops and other problems in cfg80211

[Luis R. Rodriguez]

On Mon, Mar 16, 2009 at 10:23 AM, Quentin Armitage
<Quentin@armitage.org.uk> wrote:
> If cfg80211 is compiled with CONFIG_WIRELESS_OLD_REGULATORY defined, if
> insmod is loaded with "insmod cfg80211 ieee80211_regdom=EU" then the
> execution of insmod mac80211, executing insmod ath5k causes an oops.
> This is caused by wiphy_update_regulatory being called when last_request
> == NULL, and the call of reg_is_world_roaming (via reg_process_beacons)
> causes the oops at last_request->initiator != REGDOM_SET_BY_COUNTRY_IE.
>
> If reg_is_world_roaming is modified to check for last_request not being
> NULL, e.g.
> if (last_request && last_request->initiator != REGDOM_SET_BY_COUNTRY_IE
> &&
> then I also get an oops in reg_device_remove where last_request is
> referenced and there is only a subsequent check for it being NULL.
>
> Even if the above two are fixed, it will not associate with the access
> point, and `iwlist wlan0 channels` produces a rather confused list. It
> appears that, despite the comment, in regulatory_init it is necessary to
> call regulatory_hint_core even for the pseudo country EU.

Yes, good catch, the entire code in reg.c assumes this was always
done. We overlooked the fact when "EU" became the only exemption.

> Patch1 below covers the points above, although I am not sure how useful
> the BUG_ON (!last_request) in reg_is_world_roaming is.

Its better to understand the code than sprinkle random BUG_ONs, and in
this particular case you've already found the issue. So lets address
and fix that appropriately.

> Having modified the above, I then found that the regulatory domain was
> not being updated from the Country IE sent by the access point.
>  It
> appears that the code following the comment "so we optimize an early
> check ..." in regulatory_hint_11d is a little over optimized, since if
> the following if statement is true, every path through that block ends
> in "goto out;". Patch 2 below allows the Country IE to be processed
> (note it also needs an extra check around a WARN_ON, and so the
> preceeding comment to that needs considering).

Thanks for reporting this as well, as you noted we didn't check for
the last_request type and just made an assumption. The check should go
on earlier.

Will spin some patches to address this. The right solution really is
to abandon OLD_REG, but that won't happen for stable right now so will
spin some patches to address this for stable.

Thanks,

  Luis

Re: Oops and other problems in cfg80211

[Quentin Armitage]

Luis R. Rodriguez <mcgrof@...> writes:

> > If reg_is_world_roaming is modified to check for last_request not being
> > NULL, e.g.
> > if (last_request && last_request->initiator != REGDOM_SET_BY_COUNTRY_IE
> > &&
> > then I also get an oops in reg_device_remove where last_request is
> > referenced and there is only a subsequent check for it being NULL.
Following the commits included in master-2009-03-23, in function
reg_device_remove in net/wireless/reg.c there is still a dereference via
last_request (request_wiphy = wiphy_idx_to_wiphy(last_request->wiphy_idx);)
prior to the test for last_request != NULL (if (!last_request || !request_wiphy)
)
I suspect that following the patches applied, last_request cannot be NULL, and
therefore the check !last_request is not required; otherwise the check needs to
be made before the call to wiphy_idx_to_wiphy.
One way or the other, as it stands the code looks wrong.

Re: Oops and other problems in cfg80211

[Luis R. Rodriguez]

On Tue, Mar 24, 2009 at 3:18 PM, Quentin Armitage
<Quentin@armitage.org.uk> wrote:
> Luis R. Rodriguez <mcgrof@...> writes:
>
>> > If reg_is_world_roaming is modified to check for last_request not being
>> > NULL, e.g.
>> > if (last_request && last_request->initiator != REGDOM_SET_BY_COUNTRY_IE
>> > &&
>> > then I also get an oops in reg_device_remove where last_request is
>> > referenced and there is only a subsequent check for it being NULL.
> Following the commits included in master-2009-03-23, in function
> reg_device_remove in net/wireless/reg.c there is still a dereference via
> last_request (request_wiphy = wiphy_idx_to_wiphy(last_request->wiphy_idx);)
> prior to the test for last_request != NULL (if (!last_request || !request_wiphy)
> )
> I suspect that following the patches applied, last_request cannot be NULL, and
> therefore the check !last_request is not required; otherwise the check needs to
> be made before the call to wiphy_idx_to_wiphy.
> One way or the other, as it stands the code looks wrong.

Heh yeah its impossible to get that but I'll send a patch. Thanks.

  Luis