Path for fixed channel issue in aircrack-ng suite

Path for fixed channel issue in aircrack-ng suite

[Joker Joker]

Below is a patch to fix monitor mode channel issue for aircrack-ng suit
(fixed channel mon0: -1)

diff --git a/net/wireless/chan.c b/net/wireless/chan.c
index d92d088..93f6c19 100644
--- a/net/wireless/chan.c
+++ b/net/wireless/chan.c
@@ -48,6 +48,7 @@ int cfg80211_set_freq(struct cfg80211_registered_device *rdev,
                      enum nl80211_channel_type channel_type)
 {
        struct ieee80211_channel *chan;
+       struct wireless_dev *old_wdev = wdev;
        int result;

        if (wdev->iftype == NL80211_IFTYPE_MONITOR)
@@ -73,8 +74,8 @@ int cfg80211_set_freq(struct cfg80211_registered_device *rdev,
        if (result)
                return result;

-       if (wdev)
-               wdev->channel = chan;
+       wdev = old_wdev;
+       wdev->channel = chan;

        return 0;
 }

Re: Path for fixed channel issue in aircrack-ng suite

[Johannes Berg]

On Tue, 2010-05-25 at 01:07 -0400, Joker Joker wrote:
> Below is a patch to fix monitor mode channel issue for aircrack-ng suit
> (fixed channel mon0: -1)
> 
> diff --git a/net/wireless/chan.c b/net/wireless/chan.c
> index d92d088..93f6c19 100644
> --- a/net/wireless/chan.c
> +++ b/net/wireless/chan.c
> @@ -48,6 +48,7 @@ int cfg80211_set_freq(struct cfg80211_registered_device *rdev,
>                       enum nl80211_channel_type channel_type)
>  {
>         struct ieee80211_channel *chan;
> +       struct wireless_dev *old_wdev = wdev;
>         int result;
> 
>         if (wdev->iftype == NL80211_IFTYPE_MONITOR)
> @@ -73,8 +74,8 @@ int cfg80211_set_freq(struct cfg80211_registered_device *rdev,
>         if (result)
>                 return result;
> 
> -       if (wdev)
> -               wdev->channel = chan;
> +       wdev = old_wdev;
> +       wdev->channel = chan;

NACK. That will crash when there really is no interface being passed in.

johannes

Re: Path for fixed channel issue in aircrack-ng suite

[Gábor Stefanik]

On Tue, May 25, 2010 at 10:11 AM, Johannes Berg
<johannes@sipsolutions.net> wrote:
> On Tue, 2010-05-25 at 01:07 -0400, Joker Joker wrote:
>> Below is a patch to fix monitor mode channel issue for aircrack-ng suit
>> (fixed channel mon0: -1)
>>
>> diff --git a/net/wireless/chan.c b/net/wireless/chan.c
>> index d92d088..93f6c19 100644
>> --- a/net/wireless/chan.c
>> +++ b/net/wireless/chan.c
>> @@ -48,6 +48,7 @@ int cfg80211_set_freq(struct cfg80211_registered_device *rdev,
>>                       enum nl80211_channel_type channel_type)
>>  {
>>         struct ieee80211_channel *chan;
>> +       struct wireless_dev *old_wdev = wdev;
>>         int result;
>>
>>         if (wdev->iftype == NL80211_IFTYPE_MONITOR)
>> @@ -73,8 +74,8 @@ int cfg80211_set_freq(struct cfg80211_registered_device *rdev,
>>         if (result)
>>                 return result;
>>
>> -       if (wdev)
>> -               wdev->channel = chan;
>> +       wdev = old_wdev;
>> +       wdev->channel = chan;
>
> NACK. That will crash when there really is no interface being passed in.
>
> johannes
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

Well, the original version already dereferences wdev in "if
(wdev->iftype...", so the crash is nothing new if it exists.

-- 
Vista: [V]iruses, [I]ntruders, [S]pyware, [T]rojans and [A]dware. :-)

Re: Path for fixed channel issue in aircrack-ng suite

[Johannes Berg]

On Wed, 2010-05-26 at 00:54 +0200, Gábor Stefanik wrote:

> >> -       if (wdev)
> >> -               wdev->channel = chan;
> >> +       wdev = old_wdev;
> >> +       wdev->channel = chan;
> >
> > NACK. That will crash when there really is no interface being passed in.
> >
> > johannes
> >
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> >
> 
> Well, the original version already dereferences wdev in "if
> (wdev->iftype...", so the crash is nothing new if it exists.

It has also been fixed since.

johannes

Re: Path for fixed channel issue in aircrack-ng suite

[Richard Farina]

Johannes Berg wrote:
> On Wed, 2010-05-26 at 00:54 +0200, Gábor Stefanik wrote:
>
>   
>>>> -       if (wdev)
>>>> -               wdev->channel = chan;
>>>> +       wdev = old_wdev;
>>>> +       wdev->channel = chan;
>>>>         
>>> NACK. That will crash when there really is no interface being passed in.
>>>
>>> johannes
>>>
>>> --
>>> To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
>>> the body of a message to majordomo@vger.kernel.org
>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>>
>>>       
>> Well, the original version already dereferences wdev in "if
>> (wdev->iftype...", so the crash is nothing new if it exists.
>>     
>
> It has also been fixed since.
>
>   
Can someone port this patch up so it includes the fix Johannes is 
speaking of? Kind of craptastic to have not one but TWO bugs which 
completely break monitor mode AND channel hopping.  If we fix both at 
once we can have a working driver :-) I have plenty of testers standing by.

Thanks,
Rick
> johannes
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
>   

Re: Path for fixed channel issue in aircrack-ng suite

[Richard Farina]

Johannes Berg wrote:
> On Wed, 2010-05-26 at 00:54 +0200, Gábor Stefanik wrote:
>
>   
>>>> -       if (wdev)
>>>> -               wdev->channel = chan;
>>>> +       wdev = old_wdev;
>>>> +       wdev->channel = chan;
>>>>         
>>> NACK. That will crash when there really is no interface being passed in.
>>>
>>> johannes
>>>
>>> --
>>> To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
>>> the body of a message to majordomo@vger.kernel.org
>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>>
>>>       
>> Well, the original version already dereferences wdev in "if
>> (wdev->iftype...", so the crash is nothing new if it exists.
>>     
>
> It has also been fixed since.
>
>   
Is someone porting this fix into wireless-testing and stable or does 
that still need to be done? Fixed channel of -1 is sort of an issue for 
using monitor mode....

Thanks,
Rick
> johannes
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
>   

Re: Path for fixed channel issue in aircrack-ng suite [V2]

[Maxim Levitsky]

While debugging recent wireless problems, I gave the aircrack suite a
shot, and found this thread.

How about this patch to fix this issue:


commit fffd6e63ea75850dafbf2ccfb38a4189f43c0282
Author: Maxim Levitsky <maximlevitsky@gmail.com>
Date:   Tue Jun 1 15:43:21 2010 +0300

    wireless: allow to retrieve the channel set on monitor interface
    
    This will allow to preserve compatibility with userspace
    
    Signed-off-by: Maxim Levitsky <maximlevitsky@gmail.com>

diff --git a/net/wireless/chan.c b/net/wireless/chan.c
index b01a6f6..09d979b 100644
--- a/net/wireless/chan.c
+++ b/net/wireless/chan.c
@@ -49,9 +49,12 @@ int cfg80211_set_freq(struct cfg80211_registered_device *rdev,
 {
 	struct ieee80211_channel *chan;
 	int result;
+	struct wireless_dev *mon_dev = NULL;
 
-	if (wdev && wdev->iftype == NL80211_IFTYPE_MONITOR)
+	if (wdev && wdev->iftype == NL80211_IFTYPE_MONITOR) {
+		mon_dev = wdev;
 		wdev = NULL;
+	}
 
 	if (wdev) {
 		ASSERT_WDEV_LOCK(wdev);
@@ -76,5 +79,8 @@ int cfg80211_set_freq(struct cfg80211_registered_device *rdev,
 	if (wdev)
 		wdev->channel = chan;
 
+	if (mon_dev)
+		mon_dev->channel = chan;
+
 	return 0;
 }

Re: Path for fixed channel issue in aircrack-ng suite [V2]

[Richard Farina]

Maxim Levitsky wrote:
> While debugging recent wireless problems, I gave the aircrack suite a
> shot, and found this thread.
>
>   
Thanks Maxim!
> How about this patch to fix this issue:
>
>   
I will definitely find some testers, the problem is that I have been 
unable to reliably reproduce the "channel -1".  Since I don't know the 
exact conditions this happens under the best I can do is have a few 
people test it and make sure no one sees the error.

Mister_X, how does this look to you? I assume you are familiar with the 
issue.

Thanks,
Rick Farina
> commit fffd6e63ea75850dafbf2ccfb38a4189f43c0282
> Author: Maxim Levitsky <maximlevitsky@gmail.com>
> Date:   Tue Jun 1 15:43:21 2010 +0300
>
>     wireless: allow to retrieve the channel set on monitor interface
>     
>     This will allow to preserve compatibility with userspace
>     
>     Signed-off-by: Maxim Levitsky <maximlevitsky@gmail.com>
>
> diff --git a/net/wireless/chan.c b/net/wireless/chan.c
> index b01a6f6..09d979b 100644
> --- a/net/wireless/chan.c
> +++ b/net/wireless/chan.c
> @@ -49,9 +49,12 @@ int cfg80211_set_freq(struct cfg80211_registered_device *rdev,
>  {
>  	struct ieee80211_channel *chan;
>  	int result;
> +	struct wireless_dev *mon_dev = NULL;
>  
> -	if (wdev && wdev->iftype == NL80211_IFTYPE_MONITOR)
> +	if (wdev && wdev->iftype == NL80211_IFTYPE_MONITOR) {
> +		mon_dev = wdev;
>  		wdev = NULL;
> +	}
>  
>  	if (wdev) {
>  		ASSERT_WDEV_LOCK(wdev);
> @@ -76,5 +79,8 @@ int cfg80211_set_freq(struct cfg80211_registered_device *rdev,
>  	if (wdev)
>  		wdev->channel = chan;
>  
> +	if (mon_dev)
> +		mon_dev->channel = chan;
> +
>  	return 0;
>  }
>
>
>
>   

Re: Path for fixed channel issue in aircrack-ng suite [V2]

[Johannes Berg]

On Tue, 2010-06-01 at 21:28 +0300, Maxim Levitsky wrote:

>     wireless: allow to retrieve the channel set on monitor interface
>     
>     This will allow to preserve compatibility with userspace
>     
>     Signed-off-by: Maxim Levitsky <maximlevitsky@gmail.com>
> 
> diff --git a/net/wireless/chan.c b/net/wireless/chan.c
> index b01a6f6..09d979b 100644
> --- a/net/wireless/chan.c
> +++ b/net/wireless/chan.c
> @@ -49,9 +49,12 @@ int cfg80211_set_freq(struct cfg80211_registered_device *rdev,
>  {
>  	struct ieee80211_channel *chan;
>  	int result;
> +	struct wireless_dev *mon_dev = NULL;
>  
> -	if (wdev && wdev->iftype == NL80211_IFTYPE_MONITOR)
> +	if (wdev && wdev->iftype == NL80211_IFTYPE_MONITOR) {
> +		mon_dev = wdev;
>  		wdev = NULL;
> +	}
>  
>  	if (wdev) {
>  		ASSERT_WDEV_LOCK(wdev);
> @@ -76,5 +79,8 @@ int cfg80211_set_freq(struct cfg80211_registered_device *rdev,
>  	if (wdev)
>  		wdev->channel = chan;
>  
> +	if (mon_dev)
> +		mon_dev->channel = chan;
> +

This won't quite work. monitor interfaces are always slaves in the sense
that their channel setting never takes precedence over anything, so if
you add another interface and do something there, the hardware will
happily channel switch away from the monitor channel, but it will still
report the old channel here that you set.

I have no idea how to _properly_ fix this though, since eventually we'll
have a situation where you can have multiple interfaces active on
different channels, so that monitors don't have a fixed channel anyway.

Maybe this patch is sufficient since it works in the case people care
about when they _only_ have a monitor interface, but I think it'd be
nicer if it wouldn't report anything in the other cases.

johannes

Re: Path for fixed channel issue in aircrack-ng suite [V2]

[Richard Farina]

Johannes Berg wrote:
> On Tue, 2010-06-01 at 21:28 +0300, Maxim Levitsky wrote:
>
>   
>>     wireless: allow to retrieve the channel set on monitor interface
>>     
>>     This will allow to preserve compatibility with userspace
>>     
>>     Signed-off-by: Maxim Levitsky <maximlevitsky@gmail.com>
>>
>> diff --git a/net/wireless/chan.c b/net/wireless/chan.c
>> index b01a6f6..09d979b 100644
>> --- a/net/wireless/chan.c
>> +++ b/net/wireless/chan.c
>> @@ -49,9 +49,12 @@ int cfg80211_set_freq(struct cfg80211_registered_device *rdev,
>>  {
>>  	struct ieee80211_channel *chan;
>>  	int result;
>> +	struct wireless_dev *mon_dev = NULL;
>>  
>> -	if (wdev && wdev->iftype == NL80211_IFTYPE_MONITOR)
>> +	if (wdev && wdev->iftype == NL80211_IFTYPE_MONITOR) {
>> +		mon_dev = wdev;
>>  		wdev = NULL;
>> +	}
>>  
>>  	if (wdev) {
>>  		ASSERT_WDEV_LOCK(wdev);
>> @@ -76,5 +79,8 @@ int cfg80211_set_freq(struct cfg80211_registered_device *rdev,
>>  	if (wdev)
>>  		wdev->channel = chan;
>>  
>> +	if (mon_dev)
>> +		mon_dev->channel = chan;
>> +
>>     
>
> This won't quite work. monitor interfaces are always slaves in the sense
> that their channel setting never takes precedence over anything, so if
> you add another interface and do something there, the hardware will
> happily channel switch away from the monitor channel, but it will still
> report the old channel here that you set.
>
> I have no idea how to _properly_ fix this though, since eventually we'll
> have a situation where you can have multiple interfaces active on
> different channels, so that monitors don't have a fixed channel anyway.
>
> Maybe this patch is sufficient since it works in the case people care
> about when they _only_ have a monitor interface, but I think it'd be
> nicer if it wouldn't report anything in the other cases.
>
>   
Is this a NACK or a "I wish there was a better way to do this but I'll 
allow it until a better way if found"?

-Rick

> johannes
>
>
>   

Re: Path for fixed channel issue in aircrack-ng suite [V2]

[Maxim Levitsky]

On Tue, 2010-06-01 at 21:19 +0200, Johannes Berg wrote: 
> On Tue, 2010-06-01 at 21:28 +0300, Maxim Levitsky wrote:
> 
> >     wireless: allow to retrieve the channel set on monitor interface
> >     
> >     This will allow to preserve compatibility with userspace
> >     
> >     Signed-off-by: Maxim Levitsky <maximlevitsky@gmail.com>
> > 
> > diff --git a/net/wireless/chan.c b/net/wireless/chan.c
> > index b01a6f6..09d979b 100644
> > --- a/net/wireless/chan.c
> > +++ b/net/wireless/chan.c
> > @@ -49,9 +49,12 @@ int cfg80211_set_freq(struct cfg80211_registered_device *rdev,
> >  {
> >  	struct ieee80211_channel *chan;
> >  	int result;
> > +	struct wireless_dev *mon_dev = NULL;
> >  
> > -	if (wdev && wdev->iftype == NL80211_IFTYPE_MONITOR)
> > +	if (wdev && wdev->iftype == NL80211_IFTYPE_MONITOR) {
> > +		mon_dev = wdev;
> >  		wdev = NULL;
> > +	}
> >  
> >  	if (wdev) {
> >  		ASSERT_WDEV_LOCK(wdev);
> > @@ -76,5 +79,8 @@ int cfg80211_set_freq(struct cfg80211_registered_device *rdev,
> >  	if (wdev)
> >  		wdev->channel = chan;
> >  
> > +	if (mon_dev)
> > +		mon_dev->channel = chan;
> > +
> 
> This won't quite work. monitor interfaces are always slaves in the sense
> that their channel setting never takes precedence over anything, so if
> you add another interface and do something there, the hardware will
> happily channel switch away from the monitor channel, but it will still
> report the old channel here that you set.
> 
> I have no idea how to _properly_ fix this though, since eventually we'll
> have a situation where you can have multiple interfaces active on
> different channels, so that monitors don't have a fixed channel anyway.
> 
> Maybe this patch is sufficient since it works in the case people care
> about when they _only_ have a monitor interface, but I think it'd be
> nicer if it wouldn't report anything in the other cases.

Why?

We don't have any wireless devices that actually support using 2
channels independently.
Therefore user should be aware that if he sets channel on one interface,
and then on other, the last setting matters.
Of course the right fix would to make channel a global property of card
rather that of interface.

Best regards,
Maxim Levitsky

Re: Path for fixed channel issue in aircrack-ng suite [V2]

[Johannes Berg]

On Tue, 2010-06-01 at 22:53 +0300, Maxim Levitsky wrote:

> > This won't quite work. monitor interfaces are always slaves in the sense
> > that their channel setting never takes precedence over anything, so if
> > you add another interface and do something there, the hardware will
> > happily channel switch away from the monitor channel, but it will still
> > report the old channel here that you set.
> > 
> > I have no idea how to _properly_ fix this though, since eventually we'll
> > have a situation where you can have multiple interfaces active on
> > different channels, so that monitors don't have a fixed channel anyway.
> > 
> > Maybe this patch is sufficient since it works in the case people care
> > about when they _only_ have a monitor interface, but I think it'd be
> > nicer if it wouldn't report anything in the other cases.
> 
> Why?
> 
> We don't have any wireless devices that actually support using 2
> channels independently.

ath9k already supports this with its virtual wiphy concept, and that
will be moved in the medium term to do channel switching between
multiple virtual interfaces. It's perfectly possible to do this with two
station interfaces, for instance, since you can pretend to be in
powersave while you go to the other channel. So we actually _do_ have
devices that support multiple channels independently, just not the way
you think of it.

> Therefore user should be aware that if he sets channel on one interface,
> and then on other, the last setting matters.

Like I said, this is not true.

> Of course the right fix would to make channel a global property of card
> rather that of interface.

No, see above ... my recent patches have moved exactly in the other
direction for this reason!

However, This obviously only works for certain special cases like 2
stations. Monitor interfaces are sort of a second class citizen, if you
have two stations on different channels and add a monitor interface, its
channel setting won't matter at all.

However, in that case, when you set a channel on the monitor interface,
with this patch it will report that channel, irregardless of what
channel it is currently on. And if you have 2 channel switching station
mode interfaces, there's no sensible channel to report at all.

Again however though, very likely you don't care about that situation.
But I still think we shouldn't report a bogus channel then. So maybe in
that situation we should just fall back to reporting nothing, i.e.
return -EINVAL?

johannes

Re: Path for fixed channel issue in aircrack-ng suite [V2]

[Johannes Berg]

On Tue, 2010-06-01 at 15:50 -0400, Richard Farina wrote:

> > This won't quite work. monitor interfaces are always slaves in the sense
> > that their channel setting never takes precedence over anything, so if
> > you add another interface and do something there, the hardware will
> > happily channel switch away from the monitor channel, but it will still
> > report the old channel here that you set.
> >
> > I have no idea how to _properly_ fix this though, since eventually we'll
> > have a situation where you can have multiple interfaces active on
> > different channels, so that monitors don't have a fixed channel anyway.
> >
> > Maybe this patch is sufficient since it works in the case people care
> > about when they _only_ have a monitor interface, but I think it'd be
> > nicer if it wouldn't report anything in the other cases.
> >
> >   
> Is this a NACK or a "I wish there was a better way to do this but I'll 
> allow it until a better way if found"?

I haven't really made up my mind. I really dislike the reporting of
wrong information though, see my other email.

johannes

Re: Path for fixed channel issue in aircrack-ng suite [V2]

[Thomas d'Otreppe]

On Tue, Jun 1, 2010 at 9:19 PM, Johannes Berg <johannes@sipsolutions.net> wrote:
> On Tue, 2010-06-01 at 15:50 -0400, Richard Farina wrote:
>
>> > This won't quite work. monitor interfaces are always slaves in the sense
>> > that their channel setting never takes precedence over anything, so if
>> > you add another interface and do something there, the hardware will
>> > happily channel switch away from the monitor channel, but it will still
>> > report the old channel here that you set.
>> >
>> > I have no idea how to _properly_ fix this though, since eventually we'll
>> > have a situation where you can have multiple interfaces active on
>> > different channels, so that monitors don't have a fixed channel anyway.
>> >
>> > Maybe this patch is sufficient since it works in the case people care
>> > about when they _only_ have a monitor interface, but I think it'd be
>> > nicer if it wouldn't report anything in the other cases.
>> >
>> >
>> Is this a NACK or a "I wish there was a better way to do this but I'll
>> allow it until a better way if found"?
>
> I haven't really made up my mind. I really dislike the reporting of
> wrong information though, see my other email.
>
> johannes
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

Reporting wrong information doesn't make sense to me and thus I also
think returning -EINVAL is a good idea in such case.

Thomas (Mister_X)