[PATCH] cfg80211: Fix changing regulatory from userspace

[PATCH] cfg80211: Fix changing regulatory from userspace

[Timo Lindhorst]

The commit de3584bd62d87b4c250129fbc46ca52c80330add -
"cfg80211: fix regulatory NULL dereference" prevents the regulatory
domain from being changed by user space. A wiphy is only present
if the request comes from driver or is set by country IE, thus
check only those cases.

Signed-off-by: Timo Lindhorst <tlnd@online.de>
---
 net/wireless/reg.c |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/net/wireless/reg.c b/net/wireless/reg.c
index 76b35df..c9588ae 100644
--- a/net/wireless/reg.c
+++ b/net/wireless/reg.c
@@ -2076,7 +2076,9 @@ static int __set_regdom(const struct ieee80211_regdomain *rd)
 	}
 
 	request_wiphy = wiphy_idx_to_wiphy(last_request->wiphy_idx);
-	if (!request_wiphy) {
+	if (((last_request->initiator == NL80211_REGDOM_SET_BY_DRIVER) ||
+	     (last_request->initiator == NL80211_REGDOM_SET_BY_COUNTRY_IE)) &&
+	    !request_wiphy) {
 		reg_set_request_processed();
 		return -ENODEV;
 	}
-- 
1.7.7.3

Re: [PATCH] cfg80211: Fix changing regulatory from userspace

[Luis R. Rodriguez]

On Tue, Nov 22, 2011 at 6:30 PM, Timo Lindhorst <tlnd@online.de> wrote:
> The commit de3584bd62d87b4c250129fbc46ca52c80330add -
> "cfg80211: fix regulatory NULL dereference" prevents the regulatory
> domain from being changed by user space. A wiphy is only present
> if the request comes from driver or is set by country IE, thus
> check only those cases.
>
> Signed-off-by: Timo Lindhorst <tlnd@online.de>

Yup but at this point I'd prefer we revert the original patch instead
given that the patch also had some other short comings. John is it too
late?

  Luis

Re: [PATCH] cfg80211: Fix changing regulatory from userspace

[John W. Linville]

On Wed, Nov 23, 2011 at 06:28:57AM -0800, Luis R. Rodriguez wrote:
> On Tue, Nov 22, 2011 at 6:30 PM, Timo Lindhorst <tlnd@online.de> wrote:
> > The commit de3584bd62d87b4c250129fbc46ca52c80330add -
> > "cfg80211: fix regulatory NULL dereference" prevents the regulatory
> > domain from being changed by user space. A wiphy is only present
> > if the request comes from driver or is set by country IE, thus
> > check only those cases.
> >
> > Signed-off-by: Timo Lindhorst <tlnd@online.de>
> 
> Yup but at this point I'd prefer we revert the original patch instead
> given that the patch also had some other short comings. John is it too
> late?

No, it isn't too late to revert it.  But can we have a new fix soon?

John
-- 
John W. Linville		Someday the world will need a hero, and you
linville@tuxdriver.com			might be all we have.  Be ready.

Re: [PATCH] cfg80211: Fix changing regulatory from userspace

[John W. Linville]

On Wed, Nov 23, 2011 at 09:34:56AM -0500, John W. Linville wrote:
> On Wed, Nov 23, 2011 at 06:28:57AM -0800, Luis R. Rodriguez wrote:
> > On Tue, Nov 22, 2011 at 6:30 PM, Timo Lindhorst <tlnd@online.de> wrote:
> > > The commit de3584bd62d87b4c250129fbc46ca52c80330add -
> > > "cfg80211: fix regulatory NULL dereference" prevents the regulatory
> > > domain from being changed by user space. A wiphy is only present
> > > if the request comes from driver or is set by country IE, thus
> > > check only those cases.
> > >
> > > Signed-off-by: Timo Lindhorst <tlnd@online.de>
> > 
> > Yup but at this point I'd prefer we revert the original patch instead
> > given that the patch also had some other short comings. John is it too
> > late?
> 
> No, it isn't too late to revert it.  But can we have a new fix soon?

Hmmm...actually, the original patch was sent in the batch yesterday.
And it was Cc: stable.  It would be a lot less confusing to merge a
correct fix on top (and Cc: stable on that too).  What is wrong with
this one?

John
-- 
John W. Linville		Someday the world will need a hero, and you
linville@tuxdriver.com			might be all we have.  Be ready.

Re: [PATCH] cfg80211: Fix changing regulatory from userspace

[Luis R. Rodriguez]

On Wed, Nov 23, 2011 at 6:42 AM, John W. Linville
<linville@tuxdriver.com> wrote:
> On Wed, Nov 23, 2011 at 09:34:56AM -0500, John W. Linville wrote:
>> On Wed, Nov 23, 2011 at 06:28:57AM -0800, Luis R. Rodriguez wrote:
>> > On Tue, Nov 22, 2011 at 6:30 PM, Timo Lindhorst <tlnd@online.de> wrote:
>> > > The commit de3584bd62d87b4c250129fbc46ca52c80330add -
>> > > "cfg80211: fix regulatory NULL dereference" prevents the regulatory
>> > > domain from being changed by user space. A wiphy is only present
>> > > if the request comes from driver or is set by country IE, thus
>> > > check only those cases.
>> > >
>> > > Signed-off-by: Timo Lindhorst <tlnd@online.de>
>> >
>> > Yup but at this point I'd prefer we revert the original patch instead
>> > given that the patch also had some other short comings. John is it too
>> > late?
>>
>> No, it isn't too late to revert it.  But can we have a new fix soon?
>
> Hmmm...actually, the original patch was sent in the batch yesterday.
> And it was Cc: stable.  It would be a lot less confusing to merge a
> correct fix on top (and Cc: stable on that too).  What is wrong with
> this one?

This one is a correct fix to one of the issues present in the original
patch, the other issue is the original patch overlooks the fact that
last_request would be left with stale data. Let me post my fixes as
RFCs on top of Johannes's original patch to clarify. Unfortunately
this doesn't have much testing but I'll post for review at least.

  Luis

Re: [PATCH] cfg80211: Fix changing regulatory from userspace

[John W. Linville]

On Wed, Nov 23, 2011 at 06:58:00AM -0800, Luis R. Rodriguez wrote:
> On Wed, Nov 23, 2011 at 6:42 AM, John W. Linville
> <linville@tuxdriver.com> wrote:
> > On Wed, Nov 23, 2011 at 09:34:56AM -0500, John W. Linville wrote:
> >> On Wed, Nov 23, 2011 at 06:28:57AM -0800, Luis R. Rodriguez wrote:
> >> > On Tue, Nov 22, 2011 at 6:30 PM, Timo Lindhorst <tlnd@online.de> wrote:
> >> > > The commit de3584bd62d87b4c250129fbc46ca52c80330add -
> >> > > "cfg80211: fix regulatory NULL dereference" prevents the regulatory
> >> > > domain from being changed by user space. A wiphy is only present
> >> > > if the request comes from driver or is set by country IE, thus
> >> > > check only those cases.
> >> > >
> >> > > Signed-off-by: Timo Lindhorst <tlnd@online.de>
> >> >
> >> > Yup but at this point I'd prefer we revert the original patch instead
> >> > given that the patch also had some other short comings. John is it too
> >> > late?
> >>
> >> No, it isn't too late to revert it.  But can we have a new fix soon?
> >
> > Hmmm...actually, the original patch was sent in the batch yesterday.
> > And it was Cc: stable.  It would be a lot less confusing to merge a
> > correct fix on top (and Cc: stable on that too).  What is wrong with
> > this one?
> 
> This one is a correct fix to one of the issues present in the original
> patch, the other issue is the original patch overlooks the fact that
> last_request would be left with stale data. Let me post my fixes as
> RFCs on top of Johannes's original patch to clarify. Unfortunately
> this doesn't have much testing but I'll post for review at least.

Looks like the RFCs got some comments.  Will there be a respin
of those?  Or should I just apply this one for now?

John
-- 
John W. Linville		Someday the world will need a hero, and you
linville@tuxdriver.com			might be all we have.  Be ready.

Re: [PATCH] cfg80211: Fix changing regulatory from userspace

[Luis R. Rodriguez]

On Mon, Nov 28, 2011 at 1:45 PM, John W. Linville
<linville@tuxdriver.com> wrote:
> On Wed, Nov 23, 2011 at 06:58:00AM -0800, Luis R. Rodriguez wrote:
>> On Wed, Nov 23, 2011 at 6:42 AM, John W. Linville
>> <linville@tuxdriver.com> wrote:
>> > On Wed, Nov 23, 2011 at 09:34:56AM -0500, John W. Linville wrote:
>> >> On Wed, Nov 23, 2011 at 06:28:57AM -0800, Luis R. Rodriguez wrote:
>> >> > On Tue, Nov 22, 2011 at 6:30 PM, Timo Lindhorst <tlnd@online.de> wrote:
>> >> > > The commit de3584bd62d87b4c250129fbc46ca52c80330add -
>> >> > > "cfg80211: fix regulatory NULL dereference" prevents the regulatory
>> >> > > domain from being changed by user space. A wiphy is only present
>> >> > > if the request comes from driver or is set by country IE, thus
>> >> > > check only those cases.
>> >> > >
>> >> > > Signed-off-by: Timo Lindhorst <tlnd@online.de>
>> >> >
>> >> > Yup but at this point I'd prefer we revert the original patch instead
>> >> > given that the patch also had some other short comings. John is it too
>> >> > late?
>> >>
>> >> No, it isn't too late to revert it.  But can we have a new fix soon?
>> >
>> > Hmmm...actually, the original patch was sent in the batch yesterday.
>> > And it was Cc: stable.  It would be a lot less confusing to merge a
>> > correct fix on top (and Cc: stable on that too).  What is wrong with
>> > this one?
>>
>> This one is a correct fix to one of the issues present in the original
>> patch, the other issue is the original patch overlooks the fact that
>> last_request would be left with stale data. Let me post my fixes as
>> RFCs on top of Johannes's original patch to clarify. Unfortunately
>> this doesn't have much testing but I'll post for review at least.
>
> Looks like the RFCs got some comments.  Will there be a respin
> of those?  Or should I just apply this one for now?

John, I'll will send a respin after I dissect what Tomas mentioned. I
have one other patch added to my series also that Rajkumar Manoharan
<rmanohar@qca.qualcomm.com> sent to me that I will be rolling into my
series. If we are pressed my recommendation is to revert Johannes'
patch for now, but if you do, please let me know so I can amend my
patches to address that bit.

I'm working on this right now.

  Luis

Re: [PATCH] cfg80211: Fix changing regulatory from userspace

[Johannes Berg]

On Mon, 2011-11-28 at 14:14 -0500, Luis R. Rodriguez wrote:

> John, I'll will send a respin after I dissect what Tomas mentioned. I
> have one other patch added to my series also that Rajkumar Manoharan
> <rmanohar@qca.qualcomm.com> sent to me that I will be rolling into my
> series. If we are pressed my recommendation is to revert Johannes'
> patch for now, but if you do, please let me know so I can amend my
> patches to address that bit.

I really don't want to revert that since it fixes a NULL pointer
dereference that you can probably trigger fairly easily from userspace
with not much more than well-timed unplug and lots of disk activity.

Why can't you just work on the fix first and then do all those
improvement patches?

johannes