ath9k: fix access to freed data on unload

PATCH In review Pavel Roskin 3 participants 5 messages Jan 31, 2010 – Feb 1, 2010

Subsystems (2)

Qualcomm Atheros ATH9K wireless driver Atheros ath generic utilities

lore

Share & tools

lore

Pavel Roskin

author

2010-01-31 02:37 UTC #0 lore raw

PATCH 1/1 ath9k: fix access to freed data on unload

Calling ath_bus_cleanup() after ieee80211_free_hw() resulted in access to common->bus_ops, which is already freed as part of the device data.

Remove the cleanup field in struct ath_bus_ops, as it was never used properly. Remove ath_bus_cleanup(). Merge cleanup functions in place of the ath_bus_cleanup() calls. Take care not to use any device data after ieee80211_free_hw().

Signed-off-by Pavel Roskin <[email protected]>

---

The bug was causing a hang on a kernel with most debugging options enabled. I think the fix is important and simple enough for stable kernels. I wish I could make the patch smaller, but I didn't want to leave unused and dangerous fields and functions.

ath9k was tested on the PCI bus. ath9k on AHB was compile tested. ath5k and ar9170 use struct ath_bus_ops, but don't use the cleanup field, so they are not affected.

 drivers/net/wireless/ath/ath.h         |    1 -
 drivers/net/wireless/ath/ath9k/ahb.c   |   12 ++----------
 drivers/net/wireless/ath/ath9k/ath9k.h |    5 -----
 drivers/net/wireless/ath/ath9k/pci.c   |   18 +++++-------------
 4 files changed, 7 insertions(+), 29 deletions(-)

drivers/net/wireless/ath/ath.h +0 -1

index 9e05648..71fc960 100644
@@ -74,7 +74,6 @@ struct ath_common;
 
 struct ath_bus_ops {
 	void		(*read_cachesize)(struct ath_common *common, int *csz);
-	void		(*cleanup)(struct ath_common *common);
 	bool		(*eeprom_read)(struct ath_common *common, u32 off, u16 *data);
 	void		(*bt_coex_prep)(struct ath_common *common);
 };

drivers/net/wireless/ath/ath9k/ahb.c +2 ~~-10~~

index 9e62a56..ca4994f 100644
@@ -27,12 +27,6 @@ static void ath_ahb_read_cachesize(struct ath_common *common, int *csz)
 	*csz = L1_CACHE_BYTES >> 2;
 }
 
-static void ath_ahb_cleanup(struct ath_common *common)
-{
-	struct ath_softc *sc = (struct ath_softc *)common->priv;
-	iounmap(sc->mem);
-}
-
 static bool ath_ahb_eeprom_read(struct ath_common *common, u32 off, u16 *data)
 {
 	struct ath_softc *sc = (struct ath_softc *)common->priv;

@@ -54,8 +48,6 @@ static bool ath_ahb_eeprom_read(struct ath_common *common, u32 off, u16 *data)
 
 static struct ath_bus_ops ath_ahb_bus_ops  = {
 	.read_cachesize = ath_ahb_read_cachesize,
-	.cleanup = ath_ahb_cleanup,
-
 	.eeprom_read = ath_ahb_eeprom_read,
 };

@@ -164,12 +156,12 @@ static int ath_ahb_remove(struct platform_device *pdev)
 	if (hw) {
 		struct ath_wiphy *aphy = hw->priv;
 		struct ath_softc *sc = aphy->sc;
-		struct ath_common *common = ath9k_hw_common(sc->sc_ah);
+		void __iomem *mem = sc->mem;
 
 		ath9k_deinit_device(sc);
 		free_irq(sc->irq, sc);
 		ieee80211_free_hw(sc->hw);
-		ath_bus_cleanup(common);
+		iounmap(mem);
 		platform_set_drvdata(pdev, NULL);
 	}

drivers/net/wireless/ath/ath9k/ath9k.h +0 -5

index 3f8a7e7..0ea340f 100644
@@ -538,11 +538,6 @@ static inline void ath_read_cachesize(struct ath_common *common, int *csz)
 	common->bus_ops->read_cachesize(common, csz);
 }
 
-static inline void ath_bus_cleanup(struct ath_common *common)
-{
-	common->bus_ops->cleanup(common);
-}
-
 extern struct ieee80211_ops ath9k_ops;
 extern int modparam_nohwcrypt;

drivers/net/wireless/ath/ath9k/pci.c +5 ~~-13~~

index 4ae7b5f..f2afcbe 100644
@@ -49,16 +49,6 @@ static void ath_pci_read_cachesize(struct ath_common *common, int *csz)
 		*csz = DEFAULT_CACHELINE >> 2;   /* Use the default size */
 }
 
-static void ath_pci_cleanup(struct ath_common *common)
-{
-	struct ath_softc *sc = (struct ath_softc *) common->priv;
-	struct pci_dev *pdev = to_pci_dev(sc->dev);
-
-	pci_iounmap(pdev, sc->mem);
-	pci_disable_device(pdev);
-	pci_release_region(pdev, 0);
-}
-
 static bool ath_pci_eeprom_read(struct ath_common *common, u32 off, u16 *data)
 {
 	struct ath_hw *ah = (struct ath_hw *) common->ah;

@@ -98,7 +88,6 @@ static void ath_pci_bt_coex_prep(struct ath_common *common)
 
 static const struct ath_bus_ops ath_pci_bus_ops = {
 	.read_cachesize = ath_pci_read_cachesize,
-	.cleanup = ath_pci_cleanup,
 	.eeprom_read = ath_pci_eeprom_read,
 	.bt_coex_prep = ath_pci_bt_coex_prep,
 };

@@ -245,12 +234,15 @@ static void ath_pci_remove(struct pci_dev *pdev)
 	struct ieee80211_hw *hw = pci_get_drvdata(pdev);
 	struct ath_wiphy *aphy = hw->priv;
 	struct ath_softc *sc = aphy->sc;
-	struct ath_common *common = ath9k_hw_common(sc->sc_ah);
+	void __iomem *mem = sc->mem;
 
 	ath9k_deinit_device(sc);
 	free_irq(sc->irq, sc);
 	ieee80211_free_hw(sc->hw);
-	ath_bus_cleanup(common);
+
+	pci_iounmap(pdev, mem);
+	pci_disable_device(pdev);
+	pci_release_region(pdev, 0);
 }
 
 #ifdef CONFIG_PM

Signature · 2 lines

Regards,
Pavel Roskin

Luis R. Rodriguez

2010-02-01 18:12 UTC #1 lore raw

Quoted 11 lines · Pavel Roskin

Calling ath_bus_cleanup() after ieee80211_free_hw() resulted in access to common->bus_ops, which is already freed as part of the device data.
Remove the cleanup field in struct ath_bus_ops, as it was never used properly. Remove ath_bus_cleanup(). Merge cleanup functions in place of the ath_bus_cleanup() calls. Take care not to use any device data after ieee80211_free_hw().
Signed-off-by Pavel Roskin <[email protected]>
---

Quoted 8 lines

The bug was causing a hang on a kernel with most debugging options enabled. I think the fix is important and simple enough for stable kernels. I wish I could make the patch smaller, but I didn't want to leave unused and dangerous fields and functions.
ath9k was tested on the PCI bus. ath9k on AHB was compile tested. ath5k and ar9170 use struct ath_bus_ops, but don't use the cleanup field, so they are not affected.

Thanks, can you please resend with a Cc: [email protected] on the commit log?

Luis

John W. Linville

2010-02-01 20:45 UTC #2 lore raw

Quoted 2 lines · Luis R. Rodriguez

Quoted 1 line · Pavel Roskin
Calling ath_bus_cleanup() after ieee80211_free_hw() resulted in acc=

ess

Quoted 1 line

Quoted 1 line
to common->bus_ops, which is already freed as part of the device da=

ta.

Quoted 2 lines

Quoted 2 lines
Remove the cleanup field in struct ath_bus_ops, as it was never use=

Quoted 1 line

Quoted 1 line
properly. =A0Remove ath_bus_cleanup(). =A0Merge cleanup functions i=

n place

Quoted 1 line

Quoted 1 line
of the ath_bus_cleanup() calls. =A0Take care not to use any device =

data

Quoted 7 lines

Quoted 7 lines
after ieee80211_free_hw().
Signed-off-by Pavel Roskin <[email protected]>
---
The bug was causing a hang on a kernel with most debugging options enabled. =A0I think the fix is important and simple enough for stab=

Quoted 1 line

Quoted 1 line
kernels. =A0I wish I could make the patch smaller, but I didn't wan=

t to

Quoted 3 lines

Quoted 3 lines
leave unused and dangerous fields and functions.
ath9k was tested on the PCI bus. =A0ath9k on AHB was compile tested=

=2E

Quoted 4 lines

Quoted 2 lines
ath5k and ar9170 use struct ath_bus_ops, but don't use the cleanup field, so they are not affected.
=20
Thanks, can you please resend with a Cc: [email protected] on the com=

mit log?

Doesn't look to me like the patch even applies to 2.6.33...?

--=20
John W. Linville Someday the world will need a hero, and you
[email protected] might be all we have. Be ready.
--
To unsubscribe from this list: send the line "unsubscribe linux-wireles= s" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Luis R. Rodriguez

2010-02-01 21:32 UTC #3 lore raw

Quoted 18 lines · John W. Linville

Quoted 17 lines · Luis R. Rodriguez
Quoted 16 lines · Pavel Roskin
Calling ath_bus_cleanup() after ieee80211_free_hw() resulted in access to common->bus_ops, which is already freed as part of the device data.
Remove the cleanup field in struct ath_bus_ops, as it was never used properly. Remove ath_bus_cleanup(). Merge cleanup functions in place of the ath_bus_cleanup() calls. Take care not to use any device data after ieee80211_free_hw().
Signed-off-by Pavel Roskin <[email protected]>
---
The bug was causing a hang on a kernel with most debugging options enabled. I think the fix is important and simple enough for stable kernels. I wish I could make the patch smaller, but I didn't want to leave unused and dangerous fields and functions.

Quoted 7 lines

Quoted 5 lines
Quoted 3 lines
ath9k was tested on the PCI bus. ath9k on AHB was compile tested. ath5k and ar9170 use struct ath_bus_ops, but don't use the cleanup field, so they are not affected.
Thanks, can you please resend with a Cc: [email protected] on the commit log?
Doesn't look to me like the patch even applies to 2.6.33...?

Sorry thought it would have by then.

Luis

Pavel Roskin

author

2010-02-01 21:59 UTC #4 lore raw

Quoted 5 lines · Luis R. Rodriguez

Quoted 3 lines
Quoted 1 line
Thanks, can you please resend with a Cc: [email protected] on the commit log?
Doesn't look to me like the patch even applies to 2.6.33...?
Sorry thought it would have by then.

The current mainline 2.6.33 uses the cleanup field, but in a different way. ath_bus_cleanup() is called before ieee80211_free_hw(), so it's should not be affected by the bug. I was wrong to assume that 2.6.33 is affected.

As for wireless-testing, once ath_bus_cleanup() moved from main.c to ahb.c and pci.c, it became redundant (apart from the buggy part).

If we ever want to factorize some cleanup code in the future, it will be different code anyway, so the current "cleanup" should go away.

Signature · 2 lines

Regards,
Pavel Roskin