[PATCH v3 1/2] cfg80211: fix regulatory restore upon user hints

[PATCH v3 1/2] cfg80211: fix regulatory restore upon user hints

[Luis R. Rodriguez]

When we restore regulatory settings its possible CRDA
will not reply because of a bogus user entry. In this
case the bogus entry will prevent any further processing
on cfg80211 for regulatory domains even if we restore
regulatory settings.

To prevent this we suck out all pending requests when
restoring regulatory settings and add them back into the
queue after we have queued up the reset work.

The impact of not having this applied is that a user
with privileges can issue a userspace regulatory hint
while we are disasocciating and this would prevent any
further processing of regulatory domains.

Signed-off-by: Luis R. Rodriguez <lrodriguez@atheros.com>
---

John, since this doesn't kill any kittens I didn't mark this
as stable but I'll leave it to you to decide, I tried to describe
the impact as best as possible. Let me know if you have any
questions.

 net/wireless/reg.c |   46 +++++++++++++++++++++++++++++++++++++++++++++-
 1 files changed, 45 insertions(+), 1 deletions(-)

diff --git a/net/wireless/reg.c b/net/wireless/reg.c
index 3332d5b..e759204 100644
--- a/net/wireless/reg.c
+++ b/net/wireless/reg.c
@@ -1744,6 +1744,8 @@ static void restore_regulatory_settings(bool reset_user)
 {
 	char alpha2[2];
 	struct reg_beacon *reg_beacon, *btmp;
+	struct regulatory_request *reg_request, *tmp;
+	static LIST_HEAD(tmp_reg_req_list);
 
 	mutex_lock(&cfg80211_mutex);
 	mutex_lock(&reg_mutex);
@@ -1751,6 +1753,25 @@ static void restore_regulatory_settings(bool reset_user)
 	reset_regdomains();
 	restore_alpha2(alpha2, reset_user);
 
+	/*
+	 * If there's any pending requests we simply
+	 * stash them to a temporary pending queue and
+	 * add then after we've restored regulatory
+	 * settings.
+	 */
+	spin_lock(&reg_requests_lock);
+	if (!list_empty(&reg_requests_list)) {
+		list_for_each_entry_safe(reg_request, tmp,
+					 &reg_requests_list, list) {
+			if (reg_request->initiator !=
+			    NL80211_REGDOM_SET_BY_USER)
+				continue;
+			list_del(&reg_request->list);
+			list_add_tail(&reg_request->list, &tmp_reg_req_list);
+		}
+	}
+	spin_unlock(&reg_requests_lock);
+
 	/* Clear beacon hints */
 	spin_lock_bh(&reg_pending_beacons_lock);
 	if (!list_empty(&reg_pending_beacons)) {
@@ -1785,8 +1806,31 @@ static void restore_regulatory_settings(bool reset_user)
 	 */
 	if (is_an_alpha2(alpha2))
 		regulatory_hint_user(user_alpha2);
-}
 
+	if (list_empty(&tmp_reg_req_list))
+		return;
+
+	mutex_lock(&cfg80211_mutex);
+	mutex_lock(&reg_mutex);
+
+	spin_lock(&reg_requests_lock);
+	list_for_each_entry_safe(reg_request, tmp, &tmp_reg_req_list, list) {
+		REG_DBG_PRINT("Adding request for country %c%c back "
+			      "into the queue\n",
+			      reg_request->alpha2[0],
+			      reg_request->alpha2[1]);
+		list_del(&reg_request->list);
+		list_add_tail(&reg_request->list, &reg_requests_list);
+	}
+	spin_unlock(&reg_requests_lock);
+
+	mutex_unlock(&reg_mutex);
+	mutex_unlock(&cfg80211_mutex);
+
+	REG_DBG_PRINT("Kicking the queue\n");
+
+	schedule_work(&reg_work);
+}
 
 void regulatory_hint_disconnect(void)
 {
-- 
1.7.4.15.g7811d

Re: [PATCH v3 1/2] cfg80211: fix regulatory restore upon user hints

[Johannes Berg]

On Fri, 2011-04-01 at 12:42 -0700, Luis R. Rodriguez wrote:
> When we restore regulatory settings its possible CRDA
> will not reply because of a bogus user entry. In this
> case the bogus entry will prevent any further processing
> on cfg80211 for regulatory domains even if we restore
> regulatory settings.
> 
> To prevent this we suck out all pending requests when
> restoring regulatory settings and add them back into the
> queue after we have queued up the reset work.

What if CRDA replies in order, i.e. replies to the user requested one
first instead of the disassoc requested one?

Why do we even require crda to reply to the first in list, rather than
any one?

johannes

Re: [PATCH v3 1/2] cfg80211: fix regulatory restore upon user hints

[Luis R. Rodriguez]

On Fri, Apr 1, 2011 at 12:59 PM, Johannes Berg
<johannes@sipsolutions.net> wrote:
> On Fri, 2011-04-01 at 12:42 -0700, Luis R. Rodriguez wrote:
>> When we restore regulatory settings its possible CRDA
>> will not reply because of a bogus user entry. In this
>> case the bogus entry will prevent any further processing
>> on cfg80211 for regulatory domains even if we restore
>> regulatory settings.
>>
>> To prevent this we suck out all pending requests when
>> restoring regulatory settings and add them back into the
>> queue after we have queued up the reset work.
>
> What if CRDA replies in order, i.e. replies to the user requested one
> first instead of the disassoc requested one?

Are you questioning the order of udev events and how CRDA processes them?

If CRDA is present it should get the udev event for any valid request
and process it accordingly. If the request is bogus it'll prevent any
further processing on cfg80211 given that we simply bail out of
processing requests until last_request->processed is true. The fix for
that lies in the timeout on patch 2. This patch just ensures that we
make sure to clear out any pending requests prior to doing a restore
of regulatory settings.

> Why do we even require crda to reply to the first in list, rather than
> any one?

The order should not matter except that we want the queue to be
cleared before processing core hints when doing restoration, otherwise
the next user hint in the queue can be bogus and it will prevent a
restore.

  Luis

Re: [PATCH v3 1/2] cfg80211: fix regulatory restore upon user hints

[Johannes Berg]

On Fri, 2011-04-01 at 13:10 -0700, Luis R. Rodriguez wrote:

> > What if CRDA replies in order, i.e. replies to the user requested one
> > first instead of the disassoc requested one?
> 
> Are you questioning the order of udev events and how CRDA processes them?

No, no, not really.

> If CRDA is present it should get the udev event for any valid request
> and process it accordingly. If the request is bogus it'll prevent any
> further processing on cfg80211 given that we simply bail out of
> processing requests until last_request->processed is true. The fix for
> that lies in the timeout on patch 2. This patch just ensures that we
> make sure to clear out any pending requests prior to doing a restore
> of regulatory settings.
> 
> > Why do we even require crda to reply to the first in list, rather than
> > any one?
> 
> The order should not matter except that we want the queue to be
> cleared before processing core hints when doing restoration, otherwise
> the next user hint in the queue can be bogus and it will prevent a
> restore.

I'm just thinking this temporary clearing could cause us to reject a
reply from CRDA that's coming in at the same time that is due to a user
request.

johannes

Re: [PATCH v3 1/2] cfg80211: fix regulatory restore upon user hints

[Luis R. Rodriguez]

On Fri, Apr 1, 2011 at 1:28 PM, Johannes Berg <johannes@sipsolutions.net> wrote:
> On Fri, 2011-04-01 at 13:10 -0700, Luis R. Rodriguez wrote:
>>
>> The order should not matter except that we want the queue to be
>> cleared before processing core hints when doing restoration, otherwise
>> the next user hint in the queue can be bogus and it will prevent a
>> restore.
>
> I'm just thinking this temporary clearing could cause us to reject a
> reply from CRDA that's coming in at the same time that is due to a user
> request.

Ah, I see, hmm well I tested this with:

iw reg set US; iw reg set XA; iw reg set XB; iw reg set CR; iw dev
wlan0 disconnect; iw reg set FR;

and things were still being processed in the order sent. There is a
small race between the unlock of the reg_mutex on
restore_regulatory_settings() down until where we lock the
regulatory_hint_core() and regulatory_hint_user(). The chances of a
user regulatory hint coming in between is very low, I could not do it.
Then there is also a small race of getting a user reg hint on
restore_regulatory_settings() in between the regulatory_hint_user()
and the lock of the reg_mutex again for processing old regulatory
hints, but the worst that can happen in that case is we squeeze in a
user regulatory hint in before the other older regulatory hints, and
this is fine.

I cannot see any other cases here where we'd block a request from userspace.

  Luis

Re: [PATCH v3 1/2] cfg80211: fix regulatory restore upon user hints

[Johannes Berg]

On Fri, 2011-04-01 at 14:47 -0700, Luis R. Rodriguez wrote:
> On Fri, Apr 1, 2011 at 1:28 PM, Johannes Berg <johannes@sipsolutions.net> wrote:
> > On Fri, 2011-04-01 at 13:10 -0700, Luis R. Rodriguez wrote:
> >>
> >> The order should not matter except that we want the queue to be
> >> cleared before processing core hints when doing restoration, otherwise
> >> the next user hint in the queue can be bogus and it will prevent a
> >> restore.
> >
> > I'm just thinking this temporary clearing could cause us to reject a
> > reply from CRDA that's coming in at the same time that is due to a user
> > request.
> 
> Ah, I see, hmm well I tested this with:
> 
> iw reg set US; iw reg set XA; iw reg set XB; iw reg set CR; iw dev
> wlan0 disconnect; iw reg set FR;

was CRDA running though? If so, it will probably reply right away -- I
don't think you can hit the race easily.

> and things were still being processed in the order sent. There is a
> small race between the unlock of the reg_mutex on
> restore_regulatory_settings() down until where we lock the
> regulatory_hint_core() and regulatory_hint_user(). The chances of a
> user regulatory hint coming in between is very low, I could not do it.
> Then there is also a small race of getting a user reg hint on
> restore_regulatory_settings() in between the regulatory_hint_user()
> and the lock of the reg_mutex again for processing old regulatory
> hints, but the worst that can happen in that case is we squeeze in a
> user regulatory hint in before the other older regulatory hints, and
> this is fine.
> 
> I cannot see any other cases here where we'd block a request from userspace.

No not block a request. I'm just thinking that this taking things off
the list temporarily might erroneously discard a crda response.

johannes

Re: [PATCH v3 1/2] cfg80211: fix regulatory restore upon user hints

[Luis R. Rodriguez]

On Fri, Apr 1, 2011 at 2:49 PM, Johannes Berg <johannes@sipsolutions.net> wrote:
> On Fri, 2011-04-01 at 14:47 -0700, Luis R. Rodriguez wrote:
>> On Fri, Apr 1, 2011 at 1:28 PM, Johannes Berg <johannes@sipsolutions.net> wrote:
>> > On Fri, 2011-04-01 at 13:10 -0700, Luis R. Rodriguez wrote:
>> >>
>> >> The order should not matter except that we want the queue to be
>> >> cleared before processing core hints when doing restoration, otherwise
>> >> the next user hint in the queue can be bogus and it will prevent a
>> >> restore.
>> >
>> > I'm just thinking this temporary clearing could cause us to reject a
>> > reply from CRDA that's coming in at the same time that is due to a user
>> > request.
>>
>> Ah, I see, hmm well I tested this with:
>>
>> iw reg set US; iw reg set XA; iw reg set XB; iw reg set CR; iw dev
>> wlan0 disconnect; iw reg set FR;
>
> was CRDA running though?

Yes

> If so, it will probably reply right away

Except for the cases that are bogus: XA, XB

> -- I  don't think you can hit the race easily.

I'm just trying to see the entry points on the code for such a race,
apart from the points I mentioned I cannot see any other case. Can
you?

>> and things were still being processed in the order sent. There is a
>> small race between the unlock of the reg_mutex on
>> restore_regulatory_settings() down until where we lock the
>> regulatory_hint_core() and regulatory_hint_user(). The chances of a
>> user regulatory hint coming in between is very low, I could not do it.
>> Then there is also a small race of getting a user reg hint on
>> restore_regulatory_settings() in between the regulatory_hint_user()
>> and the lock of the reg_mutex again for processing old regulatory
>> hints, but the worst that can happen in that case is we squeeze in a
>> user regulatory hint in before the other older regulatory hints, and
>> this is fine.
>>
>> I cannot see any other cases here where we'd block a request from userspace.
>
> No not block a request. I'm just thinking that this taking things off
> the list temporarily might erroneously discard a crda response.

Ah, but the stuff in the queue is stuff which we have not yet sent out
to userspace. CRDA and the kernel process regulatory requests
atomically.

  Luis

Re: [PATCH v3 1/2] cfg80211: fix regulatory restore upon user hints

[Johannes Berg]

On Fri, 2011-04-01 at 15:22 -0700, Luis R. Rodriguez wrote:

> >> I cannot see any other cases here where we'd block a request from userspace.
> >
> > No not block a request. I'm just thinking that this taking things off
> > the list temporarily might erroneously discard a crda response.
> 
> Ah, but the stuff in the queue is stuff which we have not yet sent out
> to userspace. CRDA and the kernel process regulatory requests
> atomically.

Ah, ok, I misunderstood this then.

johannes

Re: [PATCH v3 1/2] cfg80211: fix regulatory restore upon user hints

[Luis R. Rodriguez]

On Mon, Apr 4, 2011 at 5:19 AM, Johannes Berg <johannes@sipsolutions.net> wrote:
> On Fri, 2011-04-01 at 15:22 -0700, Luis R. Rodriguez wrote:
>
>> >> I cannot see any other cases here where we'd block a request from userspace.
>> >
>> > No not block a request. I'm just thinking that this taking things off
>> > the list temporarily might erroneously discard a crda response.
>>
>> Ah, but the stuff in the queue is stuff which we have not yet sent out
>> to userspace. CRDA and the kernel process regulatory requests
>> atomically.
>
> Ah, ok, I misunderstood this then.

Oh OK, acked-by ?

  Luis