John, here's two stable fix patches I had mentioned. The first
one addresses a race but also makes it easier to fix the issue
Johannes had reported if request_wiphy being NULL on __set_regdom().
Since Johannes' patch is merged already this series ammends his
fix but the first patch needs to be applied in order to take advantage
of the clearing of the stale last_request through reset_regdomains().
I'm submitting these as RFCs as I have only test compiled this stuff
but we need a fix soon so sending out ASAP for review.
Luis R. Rodriguez (2):
cfg80211: fix race on init and driver registration
cfg80211: amend regulatory NULL dereference fix
net/wireless/reg.c | 34 ++++++++++++++++++++++++----------
1 files changed, 24 insertions(+), 10 deletions(-)
--
1.7.4.15.g7811d
Am Mittwoch, 23. November 2011, 16:04:46 schrieb Luis R. Rodriguez:
> There is a theoretical race that if hit will trigger
> a crash. The race is between when we issue the first
> regulatory hint, regulatory_hint_core(), gets processed
> by the workqueue and between when the first device
> gets registered to the wireless core. This is not easy
> to reproduce but it was easy to do so through the
> regulatory simulator I have been working on. This
> is a port of the fix I implemented there [1].
>
> [1]
> https://github.com/mcgrof/regsim/commit/a246ccf81f059cb662eee288aa13100f63
> 1e4cc8
>
> Cc: [email protected]
> Cc: Johannes Berg <[email protected]>
> Signed-off-by: Luis R. Rodriguez <[email protected]>
> ---
> net/wireless/reg.c | 28 ++++++++++++++++++++--------
> 1 files changed, 20 insertions(+), 8 deletions(-)
>
> diff --git a/net/wireless/reg.c b/net/wireless/reg.c
> index 76b35df..df73b96 100644
> --- a/net/wireless/reg.c
> +++ b/net/wireless/reg.c
> @@ -57,8 +57,17 @@
> #define REG_DBG_PRINT(args...)
> #endif
>
> +static struct regulatory_request core_request_world = {
> + .initiator = NL80211_REGDOM_SET_BY_CORE,
> + .alpha2[0] = '0',
> + .alpha2[1] = '0',
> + .intersect = false,
> + .processed = true,
> + .country_ie_env = ENVIRON_ANY,
> +};
> +
> /* Receipt of information from last regulatory request */
> -static struct regulatory_request *last_request;
> +static struct regulatory_request *last_request = &core_request_world;
>
> /* To trigger userspace events */
> static struct platform_device *reg_pdev;
> @@ -165,6 +174,10 @@ static void reset_regdomains(void)
>
> cfg80211_world_regdom = &world_regdom;
> cfg80211_regdomain = NULL;
> +
> + if (last_request != &core_request_world)
> + kfree(last_request);
> + last_request = &core_request_world;
> }
This breaks setting the regdom correctly! reset_regdomains() is called from
within set_regdom() (i.e. via __set_regdom()), however, the subsequent
functions called within set_regdom() expect last_request to still hold the
currently processed request:
update_all_wiphy_regulatory(last_request->initiator);
print_regdomain(cfg80211_regdomain);
nl80211_send_reg_change_event(last_request);
reg_set_request_processed();
This, for instance, prevents to set the regdom from userspace -- or precisely:
the regdom is set, but the reg_timeout worker restores the settings after the
timeout, which is not canceled as intended within reg_set_request_processed(),
since last_request->initiator is not NL80211_REGDOM_SET_BY_USER anymore as
last_request was reset to &core_request_world.
The targeted race condition is already solved by letting last_request
initially point to the static core_request_world, isn't it? I'd thus suggest
not to touch last_request from within reset_regdomains().
> @@ -1409,7 +1422,8 @@ static int __regulatory_hint(struct wiphy *wiphy,
> }
>
> new_request:
> - kfree(last_request);
> + if (last_request != &core_request_world)
> + kfree(last_request);
>
> last_request = pending_request;
> last_request->intersect = intersect;
> @@ -1579,9 +1593,6 @@ static int regulatory_hint_core(const char *alpha2)
> {
> struct regulatory_request *request;
>
> - kfree(last_request);
> - last_request = NULL;
> -
> request = kzalloc(sizeof(struct regulatory_request),
> GFP_KERNEL);
> if (!request)
> @@ -1823,6 +1834,10 @@ static void restore_regulatory_settings(bool
> reset_user) /* First restore to the basic regulatory settings */
> cfg80211_regdomain = cfg80211_world_regdom;
>
> + if (last_request != &core_request_world)
> + kfree(last_request);
> + last_request = &core_request_world;
> +
> mutex_unlock(®_mutex);
> mutex_unlock(&cfg80211_mutex);
>
> @@ -2306,9 +2321,6 @@ void /* __init_or_exit */ regulatory_exit(void)
>
> reset_regdomains();
>
> - kfree(last_request);
> -
> - last_request = NULL;
We hence need to free last_request here if necessary:
- kfree(last_request);
+ if (last_request != &core_request_world)
+ kfree(last_request);
- last_request = NULL;
Regards,
Timo
On Fri, Nov 25, 2011 at 2:59 PM, Timo Lindhorst <[email protected]> wrote:
> Am Mittwoch, 23. November 2011, 16:04:46 schrieb Luis R. Rodriguez:
>> There is a theoretical race that if hit will trigger
>> a crash. The race is between when we issue the first
>> regulatory hint, regulatory_hint_core(), gets processed
>> by the workqueue and between when the first device
>> gets registered to the wireless core. This is not easy
>> to reproduce but it was easy to do so through the
>> regulatory simulator I have been working on. This
>> is a port of the fix I implemented there [1].
>>
>> [1]
>> https://github.com/mcgrof/regsim/commit/a246ccf81f059cb662eee288aa13100f63
>> 1e4cc8
>>
>> Cc: [email protected]
>> Cc: Johannes Berg <[email protected]>
>> Signed-off-by: Luis R. Rodriguez <[email protected]>
>> ---
>> net/wireless/reg.c | 28 ++++++++++++++++++++--------
>> 1 files changed, 20 insertions(+), 8 deletions(-)
>>
>> diff --git a/net/wireless/reg.c b/net/wireless/reg.c
>> index 76b35df..df73b96 100644
>> --- a/net/wireless/reg.c
>> +++ b/net/wireless/reg.c
>> @@ -57,8 +57,17 @@
>> #define REG_DBG_PRINT(args...)
>> #endif
>>
>> +static struct regulatory_request core_request_world = {
>> + .initiator = NL80211_REGDOM_SET_BY_CORE,
>> + .alpha2[0] = '0',
>> + .alpha2[1] = '0',
>> + .intersect = false,
>> + .processed = true,
>> + .country_ie_env = ENVIRON_ANY,
>> +};
>> +
>> /* Receipt of information from last regulatory request */
>> -static struct regulatory_request *last_request;
>> +static struct regulatory_request *last_request = &core_request_world;
>>
>> /* To trigger userspace events */
>> static struct platform_device *reg_pdev;
>> @@ -165,6 +174,10 @@ static void reset_regdomains(void)
>>
>> cfg80211_world_regdom = &world_regdom;
>> cfg80211_regdomain = NULL;
>> +
>> + if (last_request != &core_request_world)
>> + kfree(last_request);
>> + last_request = &core_request_world;
>> }
> This breaks setting the regdom correctly! reset_regdomains() is called from
> within set_regdom() (i.e. via __set_regdom()), however, the subsequent
> functions called within set_regdom() expect last_request to still hold the
> currently processed request:
>
> update_all_wiphy_regulatory(last_request->initiator);
> print_regdomain(cfg80211_regdomain);
> nl80211_send_reg_change_event(last_request);
> reg_set_request_processed();
>
> This, for instance, prevents to set the regdom from userspace -- or precisely:
> the regdom is set, but the reg_timeout worker restores the settings after the
> timeout, which is not canceled as intended within reg_set_request_processed(),
> since last_request->initiator is not NL80211_REGDOM_SET_BY_USER anymore as
> last_request was reset to &core_request_world.
Thanks for testing, I really didn't have time. So I'll fix this by
passing a flag to reset_regdomains()s. I see the issue you pointed
out. I'll test this time before sending my new series.
Luis
On Wed, 2011-11-23 at 10:04 -0500, Luis R. Rodriguez wrote:
> Johannes' patch for "cfg80211: fix regulatory NULL dereference"
> broke user regulaotry hints and it did not address the fact that
> last_request was left populated even if the previous regulatory
> hint was stale due to the wiphy disappearing.
>
> Fix user reguluatory hints by only bailing out if for those
> regulatory hints where a request_wiphy is expected. The stale last_request
> considerations are addressed through the previous fixes on last_request
> where we reset the last_request to a static world regdom request upon
> reset_regdomains(). In this case though we further enhance the effect
> by simply restoring reguluatory settings completely.
>
> Cc: [email protected]
> Cc: Johannes Berg <[email protected]>
> Signed-off-by: Luis R. Rodriguez <[email protected]>
> ---
> net/wireless/reg.c | 6 ++++--
> 1 files changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/net/wireless/reg.c b/net/wireless/reg.c
> index df73b96..6049050 100644
> --- a/net/wireless/reg.c
> +++ b/net/wireless/reg.c
> @@ -2091,8 +2091,10 @@ static int __set_regdom(const struct ieee80211_regdomain *rd)
> }
>
> request_wiphy = wiphy_idx_to_wiphy(last_request->wiphy_idx);
> - if (!request_wiphy) {
> - reg_set_request_processed();
> + if (!request_wiphy &&
> + (last_request->initiator == NL80211_REGDOM_SET_BY_DRIVER ||
> + last_request->initiator == NL80211_REGDOM_SET_BY_COUNTRY_IE)) {
> + schedule_delayed_work(®_timeout, 0);
> return -ENODEV;
This seems OK to me, but the function is really hard to follow -- maybe
(later) we should take some code duplication and make it easier to read
by switching on the type of hint first?
johannes
On Thu, Nov 24, 2011 at 10:22 AM, Johannes Berg
<[email protected]> wrote:
> On Wed, 2011-11-23 at 10:04 -0500, Luis R. Rodriguez wrote:
>> Johannes' patch for "cfg80211: fix regulatory NULL dereference"
>> broke user regulaotry hints and it did not address the fact that
>> last_request was left populated even if the previous regulatory
>> hint was stale due to the wiphy disappearing.
>>
>> Fix user reguluatory hints by only bailing out if for those
>> regulatory hints where a request_wiphy is expected. The stale last_request
>> considerations are addressed through the previous fixes on last_request
>> where we reset the last_request to a static world regdom request upon
>> reset_regdomains(). In this case though we further enhance the effect
>> by simply restoring reguluatory settings completely.
>>
>> Cc: [email protected]
>> Cc: Johannes Berg <[email protected]>
>> Signed-off-by: Luis R. Rodriguez <[email protected]>
>> ---
>> net/wireless/reg.c | 6 ++++--
>> 1 files changed, 4 insertions(+), 2 deletions(-)
>>
>> diff --git a/net/wireless/reg.c b/net/wireless/reg.c
>> index df73b96..6049050 100644
>> --- a/net/wireless/reg.c
>> +++ b/net/wireless/reg.c
>> @@ -2091,8 +2091,10 @@ static int __set_regdom(const struct ieee80211_regdomain *rd)
>> }
>>
>> request_wiphy = wiphy_idx_to_wiphy(last_request->wiphy_idx);
>> - if (!request_wiphy) {
>> - reg_set_request_processed();
>> + if (!request_wiphy &&
>> + (last_request->initiator == NL80211_REGDOM_SET_BY_DRIVER ||
>> + last_request->initiator == NL80211_REGDOM_SET_BY_COUNTRY_IE)) {
>> + schedule_delayed_work(®_timeout, 0);
>> return -ENODEV;
>
> This seems OK to me, but the function is really hard to follow -- maybe
> (later) we should take some code duplication and make it easier to read
> by switching on the type of hint first?
Agreed and good idea. I'll do this on the regsim.git for the rewrite.
Luis
There is a theoretical race that if hit will trigger
a crash. The race is between when we issue the first
regulatory hint, regulatory_hint_core(), gets processed
by the workqueue and between when the first device
gets registered to the wireless core. This is not easy
to reproduce but it was easy to do so through the
regulatory simulator I have been working on. This
is a port of the fix I implemented there [1].
[1] https://github.com/mcgrof/regsim/commit/a246ccf81f059cb662eee288aa13100f631e4cc8
Cc: [email protected]
Cc: Johannes Berg <[email protected]>
Signed-off-by: Luis R. Rodriguez <[email protected]>
---
net/wireless/reg.c | 28 ++++++++++++++++++++--------
1 files changed, 20 insertions(+), 8 deletions(-)
diff --git a/net/wireless/reg.c b/net/wireless/reg.c
index 76b35df..df73b96 100644
--- a/net/wireless/reg.c
+++ b/net/wireless/reg.c
@@ -57,8 +57,17 @@
#define REG_DBG_PRINT(args...)
#endif
+static struct regulatory_request core_request_world = {
+ .initiator = NL80211_REGDOM_SET_BY_CORE,
+ .alpha2[0] = '0',
+ .alpha2[1] = '0',
+ .intersect = false,
+ .processed = true,
+ .country_ie_env = ENVIRON_ANY,
+};
+
/* Receipt of information from last regulatory request */
-static struct regulatory_request *last_request;
+static struct regulatory_request *last_request = &core_request_world;
/* To trigger userspace events */
static struct platform_device *reg_pdev;
@@ -165,6 +174,10 @@ static void reset_regdomains(void)
cfg80211_world_regdom = &world_regdom;
cfg80211_regdomain = NULL;
+
+ if (last_request != &core_request_world)
+ kfree(last_request);
+ last_request = &core_request_world;
}
/*
@@ -1409,7 +1422,8 @@ static int __regulatory_hint(struct wiphy *wiphy,
}
new_request:
- kfree(last_request);
+ if (last_request != &core_request_world)
+ kfree(last_request);
last_request = pending_request;
last_request->intersect = intersect;
@@ -1579,9 +1593,6 @@ static int regulatory_hint_core(const char *alpha2)
{
struct regulatory_request *request;
- kfree(last_request);
- last_request = NULL;
-
request = kzalloc(sizeof(struct regulatory_request),
GFP_KERNEL);
if (!request)
@@ -1823,6 +1834,10 @@ static void restore_regulatory_settings(bool reset_user)
/* First restore to the basic regulatory settings */
cfg80211_regdomain = cfg80211_world_regdom;
+ if (last_request != &core_request_world)
+ kfree(last_request);
+ last_request = &core_request_world;
+
mutex_unlock(®_mutex);
mutex_unlock(&cfg80211_mutex);
@@ -2306,9 +2321,6 @@ void /* __init_or_exit */ regulatory_exit(void)
reset_regdomains();
- kfree(last_request);
-
- last_request = NULL;
dev_set_uevent_suppress(®_pdev->dev, true);
platform_device_unregister(reg_pdev);
--
1.7.4.15.g7811d
Johannes' patch for "cfg80211: fix regulatory NULL dereference"
broke user regulaotry hints and it did not address the fact that
last_request was left populated even if the previous regulatory
hint was stale due to the wiphy disappearing.
Fix user reguluatory hints by only bailing out if for those
regulatory hints where a request_wiphy is expected. The stale last_request
considerations are addressed through the previous fixes on last_request
where we reset the last_request to a static world regdom request upon
reset_regdomains(). In this case though we further enhance the effect
by simply restoring reguluatory settings completely.
Cc: [email protected]
Cc: Johannes Berg <[email protected]>
Signed-off-by: Luis R. Rodriguez <[email protected]>
---
net/wireless/reg.c | 6 ++++--
1 files changed, 4 insertions(+), 2 deletions(-)
diff --git a/net/wireless/reg.c b/net/wireless/reg.c
index df73b96..6049050 100644
--- a/net/wireless/reg.c
+++ b/net/wireless/reg.c
@@ -2091,8 +2091,10 @@ static int __set_regdom(const struct ieee80211_regdomain *rd)
}
request_wiphy = wiphy_idx_to_wiphy(last_request->wiphy_idx);
- if (!request_wiphy) {
- reg_set_request_processed();
+ if (!request_wiphy &&
+ (last_request->initiator == NL80211_REGDOM_SET_BY_DRIVER ||
+ last_request->initiator == NL80211_REGDOM_SET_BY_COUNTRY_IE)) {
+ schedule_delayed_work(®_timeout, 0);
return -ENODEV;
}
--
1.7.4.15.g7811d