ath9k: Prevent divide by zero kernel crash.

PATCH New greearb at candelatech.com 3 participants 5 messages Apr 17, 2014 – Apr 30, 2014 Qualcomm Atheros ATH9K wireless driver

lore

Share & tools

lore

greearb at candelatech.com

author

2014-04-17 00:40 UTC #0 lore raw

PATCH 1/1 ath9k: Prevent divide by zero kernel crash.

From: Ben Greear <[email protected]>

Make sure we cannot ever assign beacon interval to zero.

Signed-off-by Ben Greear <[email protected]>

---
 drivers/net/wireless/ath/ath9k/beacon.c | 4 ++++
 drivers/net/wireless/ath/ath9k/recv.c   | 3 ++-
 2 files changed, 6 insertions(+), 1 deletion(-)

drivers/net/wireless/ath/ath9k/beacon.c +4 -0

index 2e8bba0..5391f01 100644
@@ -443,6 +443,8 @@ static u32 ath9k_mod_tsf64_tu(u64 tsf, u32 div_tu)
 {
 	u32 tsf_mod, tsf_hi, tsf_lo, mod_hi, mod_lo;
 
+	if (WARN_ON_ONCE(div_tu == 0))
+		div_tu = 100;
 	tsf_mod = tsf & (BIT(10) - 1);
 	tsf_hi = tsf >> 32;
 	tsf_lo = ((u32) tsf) >> 10;

@@ -667,6 +669,8 @@ static void ath9k_cache_beacon_config(struct ath_softc *sc,
 		"Caching beacon data for BSS: %pM\n", bss_conf->bssid);
 
 	cur_conf->beacon_interval = bss_conf->beacon_int;
+	if (WARN_ON_ONCE(cur_conf->beacon_interval == 0))
+		cur_conf->beacon_interval = 100;
 	cur_conf->dtim_period = bss_conf->dtim_period;
 	cur_conf->listen_interval = 1;
 	cur_conf->dtim_count = 1;

drivers/net/wireless/ath/ath9k/recv.c +2 -1

index b97217d..79c20c7 100644
@@ -538,7 +538,8 @@ static void ath_rx_ps_beacon(struct ath_softc *sc, struct sk_buff *skb)
 		sc->ps_flags &= ~PS_BEACON_SYNC;
 		ath_dbg(common, PS,
 			"Reconfigure beacon timers based on synchronized timestamp\n");
-		ath9k_set_beacon(sc);
+		if (!(WARN_ON_ONCE(sc->cur_beacon_conf.beacon_interval == 0)))
+			ath9k_set_beacon(sc);
 	}
 
 	if (ath_beacon_dtim_pending_cab(skb)) {

Signature · 1 line

1.7.11.7

Felix Fietkau

2014-04-17 07:47 UTC #1 lore raw

PATCH 1/1 ath9k: Prevent divide by zero kernel crash.

Quoted 15 lines · greearb at candelatech.com

From: Ben Greear <[email protected]>
Make sure we cannot ever assign beacon interval to zero.
Signed-off-by Ben Greear <[email protected]>
---
 drivers/net/wireless/ath/ath9k/beacon.c | 4 ++++
 drivers/net/wireless/ath/ath9k/recv.c   | 3 ++-
 2 files changed, 6 insertions(+), 1 deletion(-)
drivers/net/wireless/ath/ath9k/beacon.c +0 -0
index 2e8bba0..5391f01 100644

drivers/net/wireless/ath/ath9k/beacon.c static u32 ath9k_mod_tsf64_tu(u64 tsf, u32 div_tu) · line 450

Quoted 8 lines

drivers/net/wireless/ath/ath9k/beacon.c +2 -0

@@ -443,6 +443,8 @@ static u32 ath9k_mod_tsf64_tu(u64 tsf, u32 div_tu)
 {
 	u32 tsf_mod, tsf_hi, tsf_lo, mod_hi, mod_lo;
 
+	if (WARN_ON_ONCE(div_tu == 0))
+		div_tu = 100;
 	tsf_mod = tsf & (BIT(10) - 1);
 	tsf_hi = tsf >> 32;
 	tsf_lo = ((u32) tsf) >> 10;

Why add this warning here if you already have the additions below? We don't need multiple layers of defensive checks for the same thing.

Quoted 24 lines

drivers/net/wireless/ath/ath9k/beacon.c +2 -0

@@ -667,6 +669,8 @@ static void ath9k_cache_beacon_config(struct ath_softc *sc,
 		"Caching beacon data for BSS: %pM\n", bss_conf->bssid);
 
 	cur_conf->beacon_interval = bss_conf->beacon_int;
+	if (WARN_ON_ONCE(cur_conf->beacon_interval == 0))
+		cur_conf->beacon_interval = 100;
 	cur_conf->dtim_period = bss_conf->dtim_period;
 	cur_conf->listen_interval = 1;
 	cur_conf->dtim_count = 1;

drivers/net/wireless/ath/ath9k/recv.c +2 -1

index b97217d..79c20c7 100644
@@ -538,7 +538,8 @@ static void ath_rx_ps_beacon(struct ath_softc *sc, struct sk_buff *skb)
 		sc->ps_flags &= ~PS_BEACON_SYNC;
 		ath_dbg(common, PS,
 			"Reconfigure beacon timers based on synchronized timestamp\n");
-		ath9k_set_beacon(sc);
+		if (!(WARN_ON_ONCE(sc->cur_beacon_conf.beacon_interval == 0)))
+			ath9k_set_beacon(sc);
 	}
 
 	if (ath_beacon_dtim_pending_cab(skb)) {

John W. Linville

2014-04-22 17:53 UTC #2 lore raw

PATCH 1/1 ath9k: Prevent divide by zero kernel crash.

Please consider Felix's comments while refactoring this for the current code structure...

John

Quoted 57 lines · Felix Fietkau

Quoted 15 lines · greearb at candelatech.com
From: Ben Greear <[email protected]>
Make sure we cannot ever assign beacon interval to zero.
Signed-off-by Ben Greear <[email protected]>
---
 drivers/net/wireless/ath/ath9k/beacon.c | 4 ++++
 drivers/net/wireless/ath/ath9k/recv.c   | 3 ++-
 2 files changed, 6 insertions(+), 1 deletion(-)
drivers/net/wireless/ath/ath9k/beacon.c +0 -0
index 2e8bba0..5391f01 100644
drivers/net/wireless/ath/ath9k/beacon.c static u32 ath9k_mod_tsf64_tu(u64 tsf, u32 div_tu) · line 450
Quoted 8 lines
drivers/net/wireless/ath/ath9k/beacon.c +2 -0
@@ -443,6 +443,8 @@ static u32 ath9k_mod_tsf64_tu(u64 tsf, u32 div_tu)
 {
 	u32 tsf_mod, tsf_hi, tsf_lo, mod_hi, mod_lo;
 
+	if (WARN_ON_ONCE(div_tu == 0))
+		div_tu = 100;
 	tsf_mod = tsf & (BIT(10) - 1);
 	tsf_hi = tsf >> 32;
 	tsf_lo = ((u32) tsf) >> 10;
Why add this warning here if you already have the additions below? We don't need multiple layers of defensive checks for the same thing.
Quoted 15 lines
drivers/net/wireless/ath/ath9k/beacon.c +2 -0
@@ -667,6 +669,8 @@ static void ath9k_cache_beacon_config(struct ath_softc *sc,
 		"Caching beacon data for BSS: %pM\n", bss_conf->bssid);
 
 	cur_conf->beacon_interval = bss_conf->beacon_int;
+	if (WARN_ON_ONCE(cur_conf->beacon_interval == 0))
+		cur_conf->beacon_interval = 100;
 	cur_conf->dtim_period = bss_conf->dtim_period;
 	cur_conf->listen_interval = 1;
 	cur_conf->dtim_count = 1;
drivers/net/wireless/ath/ath9k/recv.c +0 -0
index b97217d..79c20c7 100644
drivers/net/wireless/ath/ath9k/recv.c static void ath_rx_ps_beacon(struct ath_softc *sc, struct sk_buff *skb) · line 545
Quoted 9 lines
drivers/net/wireless/ath/ath9k/recv.c +2 -1
@@ -538,7 +538,8 @@ static void ath_rx_ps_beacon(struct ath_softc *sc, struct sk_buff *skb)
 		sc->ps_flags &= ~PS_BEACON_SYNC;
 		ath_dbg(common, PS,
 			"Reconfigure beacon timers based on synchronized timestamp\n");
-		ath9k_set_beacon(sc);
+		if (!(WARN_ON_ONCE(sc->cur_beacon_conf.beacon_interval == 0)))
+			ath9k_set_beacon(sc);
 	}
 
 	if (ath_beacon_dtim_pending_cab(skb)) {
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo at vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

Signature · 2 lines

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

Ben Greear

author

2014-04-22 18:03 UTC #3 lore raw

PATCH 1/1 ath9k: Prevent divide by zero kernel crash.

Quoted 2 lines · John W. Linville

Please consider Felix's comments while refactoring this for the current code structure...

I'm low on time, but will resend a patch when I get a chance.

If someone else wants to send in the updated patch that is fine by me.

Thanks,
Ben

Quoted 62 lines

John
Quoted 57 lines · Felix Fietkau
Quoted 15 lines · greearb at candelatech.com
From: Ben Greear <[email protected]>
Make sure we cannot ever assign beacon interval to zero.
Signed-off-by Ben Greear <[email protected]>
---
 drivers/net/wireless/ath/ath9k/beacon.c | 4 ++++
 drivers/net/wireless/ath/ath9k/recv.c   | 3 ++-
 2 files changed, 6 insertions(+), 1 deletion(-)
drivers/net/wireless/ath/ath9k/beacon.c +0 -0
index 2e8bba0..5391f01 100644
drivers/net/wireless/ath/ath9k/beacon.c static u32 ath9k_mod_tsf64_tu(u64 tsf, u32 div_tu) · line 450
Quoted 8 lines
drivers/net/wireless/ath/ath9k/beacon.c +2 -0
@@ -443,6 +443,8 @@ static u32 ath9k_mod_tsf64_tu(u64 tsf, u32 div_tu)
 {
 	u32 tsf_mod, tsf_hi, tsf_lo, mod_hi, mod_lo;
 
+	if (WARN_ON_ONCE(div_tu == 0))
+		div_tu = 100;
 	tsf_mod = tsf & (BIT(10) - 1);
 	tsf_hi = tsf >> 32;
 	tsf_lo = ((u32) tsf) >> 10;
Why add this warning here if you already have the additions below? We don't need multiple layers of defensive checks for the same thing.
Quoted 15 lines
drivers/net/wireless/ath/ath9k/beacon.c +2 -0
@@ -667,6 +669,8 @@ static void ath9k_cache_beacon_config(struct ath_softc *sc,
 		"Caching beacon data for BSS: %pM\n", bss_conf->bssid);
 
 	cur_conf->beacon_interval = bss_conf->beacon_int;
+	if (WARN_ON_ONCE(cur_conf->beacon_interval == 0))
+		cur_conf->beacon_interval = 100;
 	cur_conf->dtim_period = bss_conf->dtim_period;
 	cur_conf->listen_interval = 1;
 	cur_conf->dtim_count = 1;
drivers/net/wireless/ath/ath9k/recv.c +0 -0
index b97217d..79c20c7 100644
drivers/net/wireless/ath/ath9k/recv.c static void ath_rx_ps_beacon(struct ath_softc *sc, struct sk_buff *skb) · line 545
Quoted 9 lines
drivers/net/wireless/ath/ath9k/recv.c +2 -1
@@ -538,7 +538,8 @@ static void ath_rx_ps_beacon(struct ath_softc *sc, struct sk_buff *skb)
 		sc->ps_flags &= ~PS_BEACON_SYNC;
 		ath_dbg(common, PS,
 			"Reconfigure beacon timers based on synchronized timestamp\n");
-		ath9k_set_beacon(sc);
+		if (!(WARN_ON_ONCE(sc->cur_beacon_conf.beacon_interval == 0)))
+			ath9k_set_beacon(sc);
 	}
 
 	if (ath_beacon_dtim_pending_cab(skb)) {
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo at vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

Signature · 2 lines

Ben Greear <[email protected]>
Candela Technologies Inc  http://www.candelatech.com

Ben Greear

author

2014-04-30 18:47 UTC #4 lore raw

PATCH 1/1 ath9k: Prevent divide by zero kernel crash.

In reply to Felix Fietkau

Quoted 19 lines · Felix Fietkau

Quoted 18 lines · greearb at candelatech.com
From: Ben Greear <[email protected]>
Make sure we cannot ever assign beacon interval to zero.
Signed-off-by Ben Greear <[email protected]>
---
 drivers/net/wireless/ath/ath9k/beacon.c | 4 ++++
 drivers/net/wireless/ath/ath9k/recv.c   | 3 ++-
 2 files changed, 6 insertions(+), 1 deletion(-)
drivers/net/wireless/ath/ath9k/beacon.c +0 -0
index 2e8bba0..5391f01 100644
@@ -443,6 +443,8 @@ static u32 ath9k_mod_tsf64_tu(u64 tsf, u32 div_tu)
 {
 	u32 tsf_mod, tsf_hi, tsf_lo, mod_hi, mod_lo;
 

Quoted 7 lines

drivers/net/wireless/ath/ath9k/beacon.c static u32 ath9k_mod_tsf64_tu(u64 tsf, u32 div_tu) · line 450
Quoted 5 lines
drivers/net/wireless/ath/ath9k/beacon.c +2 -0
+	if (WARN_ON_ONCE(div_tu == 0))
+		div_tu = 100;
 	tsf_mod = tsf & (BIT(10) - 1);
 	tsf_hi = tsf >> 32;
 	tsf_lo = ((u32) tsf) >> 10;
Why add this warning here if you already have the additions below? We don't need multiple layers of defensive checks for the same thing.

I am not sure I can find all cases that can send bad data to this call, and in other places, it seems having an invalid beacon interval might mess up other calculations, so better to check and set it to a better value there as well.

So, I'd prefer to leave all three warnings in, and if we ever see the one hit down in mod_tsf64_tu, then probably more protection is needed elsewhere.

Or, just treat this patch as bug report and maybe someone will fix it better...

Thanks,
Ben

Signature · 2 lines

Ben Greear <[email protected]>
Candela Technologies Inc  http://www.candelatech.com