Re: [PATCH 1/2] mac80211: minstrel, fix memory corruption

Re: [PATCH 1/2] mac80211: minstrel, fix memory corruption

[Karol Szuster]

Hi,

There is another problem with this piece of code. The sband will be NULL
after second iteration on single band device and cause null pointer
dereference. Everything is working with dual band card. Sorry, but i
don't know how to explain this clearly in English. I have looked on the
second patch for pid algorithm and found similar bug.

[PATCH] mac80211: avoid NULL ptr deref when finding max_rates in PID and minstrel

[John W. Linville]

"There is another problem with this piece of code. The sband will be NULL
after second iteration on single band device and cause null pointer
dereference. Everything is working with dual band card. Sorry, but i
don't know how to explain this clearly in English. I have looked on the
second patch for pid algorithm and found similar bug."

Reported-by: Karol Szuster <qflon@o2.pl>
Signed-off-by: John W. Linville <linville@tuxdriver.com>
---
 net/mac80211/rc80211_minstrel.c |    2 +-
 net/mac80211/rc80211_pid_algo.c |    2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/net/mac80211/rc80211_minstrel.c b/net/mac80211/rc80211_minstrel.c
index 70df3dc..d9233ec 100644
--- a/net/mac80211/rc80211_minstrel.c
+++ b/net/mac80211/rc80211_minstrel.c
@@ -477,7 +477,7 @@ minstrel_alloc_sta(void *priv, struct ieee80211_sta *sta, gfp_t gfp)
 
 	for (i = 0; i < IEEE80211_NUM_BANDS; i++) {
 		sband = hw->wiphy->bands[i];
-		if (sband->n_bitrates > max_rates)
+		if (sband && sband->n_bitrates > max_rates)
 			max_rates = sband->n_bitrates;
 	}
 
diff --git a/net/mac80211/rc80211_pid_algo.c b/net/mac80211/rc80211_pid_algo.c
index 01d59a8..8bef9a1 100644
--- a/net/mac80211/rc80211_pid_algo.c
+++ b/net/mac80211/rc80211_pid_algo.c
@@ -378,7 +378,7 @@ static void *rate_control_pid_alloc(struct ieee80211_hw *hw,
 
 	for (i = 0; i < IEEE80211_NUM_BANDS; i++) {
 		sband = hw->wiphy->bands[i];
-		if (sband->n_bitrates > max_rates)
+		if (sband && sband->n_bitrates > max_rates)
 			max_rates = sband->n_bitrates;
 	}
 
-- 
1.6.0.6

[PATCH 1/2] mac80211: minstrel, fix memory corruption

[Jiri Slaby]

minstrel doesn't count max rate count in fact, since it doesn't use
a loop variable `i' and hence allocs space only for bitrates found in
the first band.

Fix it by involving the `i' as an index so that it traverses all the
bands now and finds the real max bitrate count.

Signed-off-by: Jiri Slaby <jirislaby@gmail.com>
Cc: Felix Fietkau <nbd@openwrt.org>
---
 net/mac80211/rc80211_minstrel.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/net/mac80211/rc80211_minstrel.c b/net/mac80211/rc80211_minstrel.c
index 3824990..70df3dc 100644
--- a/net/mac80211/rc80211_minstrel.c
+++ b/net/mac80211/rc80211_minstrel.c
@@ -476,7 +476,7 @@ minstrel_alloc_sta(void *priv, struct ieee80211_sta *sta, gfp_t gfp)
 		return NULL;
 
 	for (i = 0; i < IEEE80211_NUM_BANDS; i++) {
-		sband = hw->wiphy->bands[hw->conf.channel->band];
+		sband = hw->wiphy->bands[i];
 		if (sband->n_bitrates > max_rates)
 			max_rates = sband->n_bitrates;
 	}
-- 
1.6.2.4

Re: [PATCH 1/2] mac80211: minstrel, fix memory corruption

[Bob Copeland]

On Mon, May 4, 2009 at 12:04 PM, Jiri Slaby <jirislaby@gmail.com> wrote:
> minstrel doesn't count max rate count in fact, since it doesn't use
> a loop variable `i' and hence allocs space only for bitrates found in
> the first band.

Nice catch, I don't know how many times I read that and didn't see
it.  Hopefully that has some bearing on this long standing bug:

http://bugzilla.kernel.org/show_bug.cgi?id=12490

-- 
Bob Copeland %% www.bobcopeland.com

Re: [PATCH 1/2] mac80211: minstrel, fix memory corruption

[Felix Fietkau]

Jiri Slaby wrote:
> minstrel doesn't count max rate count in fact, since it doesn't use
> a loop variable `i' and hence allocs space only for bitrates found in
> the first band.
> 
> Fix it by involving the `i' as an index so that it traverses all the
> bands now and finds the real max bitrate count.
> 
> Signed-off-by: Jiri Slaby <jirislaby@gmail.com>
> Cc: Felix Fietkau <nbd@openwrt.org>
Good catch!

Acked-by: Felix Fietkau <nbd@openwrt.org>

Re: [PATCH 1/2] mac80211: minstrel, fix memory corruption

[Luis R. Rodriguez]

On Mon, May 4, 2009 at 9:04 AM, Jiri Slaby <jirislaby@gmail.com> wrote:
> minstrel doesn't count max rate count in fact, since it doesn't use
> a loop variable `i' and hence allocs space only for bitrates found in
> the first band.
>
> Fix it by involving the `i' as an index so that it traverses all the
> bands now and finds the real max bitrate count.
>
> Signed-off-by: Jiri Slaby <jirislaby@gmail.com>
> Cc: Felix Fietkau <nbd@openwrt.org>

Cc: stable@kernel.org

  Luis

Re: [PATCH 1/2] mac80211: minstrel, fix memory corruption

[Jiri Slaby]

On 05/04/2009 08:38 PM, Luis R. Rodriguez wrote:
> Cc: stable@kernel.org

Sure, I plan to send a notification to Greg after a SHA from the linus
tree will be known.

Thanks.

Re: [PATCH 1/2] mac80211: minstrel, fix memory corruption

[Heinz Diehl]

On 04.05.2009, Jiri Slaby wrote: 

> minstrel doesn't count max rate count in fact, since it doesn't use
> a loop variable `i' and hence allocs space only for bitrates found in
> the first band.
[....]

This patchset crashes my WLAN. Reverting it does fix this:

[....]
wlan0: associated
BUG: unable to handle kernel NULL pointer dereference at 0000000000000018
IP: [<ffffffffa0273b0f>] minstrel_alloc_sta+0x6f/0xf0 [mac80211]
PGD 229da2067 PUD 229d07067 PMD 0 
Oops: 0000 [#1] PREEMPT SMP 
last sysfs file:/sys/devices/pci0000:00/0000:00:02.1/usb1/1-7/1-7:1.0/firmware/1-7:1.0/loading CPU 3 

Modules linked in: af_packet
cpufreq_conservative cpufreq_ondemand cpufreq_userspace cpufreq_powersave
powernow_k8 freq_table xt_NOTRACK ipt_REJECT xt_state iptable_raw
iptable_filter nf_conntrack_netbios_ns nf_conntrack_ipv4 nf_conntrack
nf_defrag_ipv4 ip_tables ip6_tables uhci_hcd snd_hda_codec_realtek rt73usb
rt2x00usb rt2x00lib snd_hda_intel ohci1394 snd_hda_codec led_class
ieee1394 input_polldev snd_pcm mac80211 snd_timer rtc_cmos snd ppdev
button forcedeth pcspkr firewire_ohci soundcore i2c_nforce2 rtc_core
rtc_lib parport_pc cfg80211 parport sr_mod snd_page_alloc i2c_core cdrom
sg usbhid ohci_hcd ehci_hcd sd_mod usbcore hmac loop ecb arc4 fuse
edd ext3 jbd fan pata_amd sata_nv libata scsi_mod thermal processor

Pid: 2362, comm: phy0 Not tainted 2.6.30-rc5-git5 #1  
 RIP: 0010:[<ffffffffa0273b0f>][<ffffffffa0273b0f>] minstrel_alloc_sta+0x6f/0xf0 [mac80211]
 RSP: 0018:ffff88022ddc7b90  EFLAGS: 00010206
 RAX: 000000000000000c RBX: ffff88022c150260 RCX: ffff88022c1500c0
 RDX: 0000000000000000 RSI: 0000000000008020 RDI: ffff88022b528740
 RBP: ffff88022b5286c0 R08: 0000000000000000 R09: 0000000000000058
 R10: 000000000000000c R11: ffff88022ddc7cd0 R12: 0000000000008020
 R13: 0000000000000020 R14: 0000000000000020 R15: 0000000000000000
 FS:  00007f6fc24ec6f0(0000) GS:ffff88002807f000(0000) knlGS:0000000000000000
 CS:  0010 DS: 0018 ES: 0018 CR0: 000000008005003b
 CR2: 0000000000000018 CR3: 0000000229dff000 CR4: 00000000000006e0
 DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
 DR3: 0000000000000000 DR6: 00000000ffff0ff0 DR7: 0000000000000400
 Process phy0 (pid: 2362, threadinfo ffff88022ddc6000, task ffff88022c512cd0)
 Stack: May 15 19:48:40 liesel kernel:0000000000000000 ffff88022c555000 ffff88022c150260 ffff88022f126880
  ffff88022f126600 ffffffffa025c62b ffff88022f126600 ffff88022c150260
  0000000000000053 ffff880229e18044 ffff88022f126880 ffffffffa0264670
 Call Trace:
  [<ffffffffa025c62b>] ? sta_info_alloc+0x8b/0x140 [mac80211]
  [<ffffffffa0264670>] ? ieee80211_rx_mgmt_assoc_resp+0xa20/0xb90 [mac80211]
  [<ffffffffa026f1d1>] ? __ieee80211_tx+0x61/0xd0 [mac80211]
  [<ffffffffa026f34d>] ? ieee80211_tx+0x10d/0x270 [mac80211]
  [<ffffffff8055d25a>] ? thread_return+0x3e/0x6a4
  [<ffffffffa02651f2>] ? ieee80211_sta_work+0xe2/0xab0 [mac80211]
  [<ffffffff80254246>] ? queue_work+0x26/0x60
  [<ffffffffa0265110>] ? ieee80211_sta_work+0x0/0xab0 [mac80211]
  [<ffffffff80253631>] ? worker_thread+0x141/0x230
  [<ffffffff80257c00>] ? autoremove_wake_function+0x0/0x30
  [<ffffffff802534f0>] ? worker_thread+0x0/0x230
  [<ffffffff802534f0>] ? worker_thread+0x0/0x230
  [<ffffffff802577e4>] ? kthread+0x54/0x90
  [<ffffffff8020ce2a>] ? child_rip+0xa/0x20
  [<ffffffff80257790>] ? kthread+0x0/0x90
  [<ffffffff8020ce20>] ? child_rip+0x0/0x20
 Code: 89 c5 31 c0 48 85 ed 74 6c 48 8b 4b
28 31 c0 41 b9 58 00 00 00 44 89 e6 48 8b 51 20 44 8b 52 18 45 85 d2 0f 49
42 18 48 8b 51 28 <39> 42 18 89 c3 0f 4d 5a 18 48 63 fb 49 0f af f9 e8 dc
4e 04 e0 
 RIP  [<ffffffffa0273b0f>] minstrel_alloc_sta+0x6f/0xf0 [mac80211]
 RSP <ffff88022ddc7b90>
 CR2: 0000000000000018
 ---[ end trace 7489e902c4428832 ]---
ifup-dhcp: . 
syslog-ng[3073]: last message repeated 11 times
ifup-dhcp: no IP address yet... backgrounding. 
[....]

Regards,
Heinz.

Re: [PATCH 1/2] mac80211: minstrel, fix memory corruption

[John W. Linville]

On Fri, May 15, 2009 at 08:21:31PM +0200, Heinz Diehl wrote:
> On 04.05.2009, Jiri Slaby wrote: 
> 
> > minstrel doesn't count max rate count in fact, since it doesn't use
> > a loop variable `i' and hence allocs space only for bitrates found in
> > the first band.
> [....]
> 
> This patchset crashes my WLAN. Reverting it does fix this:

http://www.kernel.org/pub/linux/kernel/people/linville/wireless-2.6/0002-mac80211-avoid-NULL-ptr-deref-when-finding-max_rate.patch

Original pull request here:

	http://marc.info/?l=linux-kernel&m=124214056932143&w=2

Unfortunately, I think Dave might have limited availability right 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 1/2] mac80211: minstrel, fix memory corruption

[Linus Torvalds]

On Fri, 15 May 2009, John W. Linville wrote:
>
> Unfortunately, I think Dave might have limited availability right now...

Yeah, I think he's on some Alaskan cruise.

Should I just pull directly from you this time?

		Linus

Re: [PATCH 1/2] mac80211: minstrel, fix memory corruption

[John W. Linville]

On Fri, May 15, 2009 at 11:49:04AM -0700, Linus Torvalds wrote:
> 
> 
> On Fri, 15 May 2009, John W. Linville wrote:
> >
> > Unfortunately, I think Dave might have limited availability right now...
> 
> Yeah, I think he's on some Alaskan cruise.
> 
> Should I just pull directly from you this time?

I think that is the best option.

Thanks!

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 1/2] mac80211: minstrel, fix memory corruption

[Marcel Holtmann]

Hi Linus,

> > Unfortunately, I think Dave might have limited availability right now...
> 
> Yeah, I think he's on some Alaskan cruise.
> 
> Should I just pull directly from you this time?

I would have a similar pull request pending for Bluetooth fixes:

	http://marc.info/?l=linux-netdev&m=124209765506433

Regards

Marcel

Re: [PATCH 1/2] mac80211: minstrel, fix memory corruption

[Linus Torvalds]

On Fri, 15 May 2009, Marcel Holtmann wrote:

> Hi Linus,
> 
> > > Unfortunately, I think Dave might have limited availability right now...
> > 
> > Yeah, I think he's on some Alaskan cruise.
> > 
> > Should I just pull directly from you this time?
> 
> I would have a similar pull request pending for Bluetooth fixes:
> 
> 	http://marc.info/?l=linux-netdev&m=124209765506433

Ok, I pulled that one too.

		Linus

Re: [PATCH 1/2] mac80211: minstrel, fix memory corruption

[Jiri Slaby]

On 05/15/2009 08:21 PM, Heinz Diehl wrote:
> This patchset crashes my WLAN. Reverting it does fix this:

Could you try this fix
http://git.kernel.org/?p=linux/kernel/git/linville/wireless-testing.git;a=commitdiff_plain;h=b3f25c62aa41e0c372536c0f3975b39f21217b57;hp=30e2341dd2c82780eddd785eb54695115c6aea8d
?