[PATCH 1/2] ath9k: fix tx99 use after free

[PATCH 1/2] ath9k: fix tx99 use after free

[miaoqing]

From: Miaoqing Pan <miaoqing@codeaurora.org>

One scenario that could lead to UAF is two threads writing
simultaneously to the "tx99" debug file. One of them would
set the "start" value to true and follow to ath9k_tx99_init().
Inside the function it would set the sc->tx99_state to true
after allocating sc->tx99skb. Then, the other thread would
execute write_file_tx99() and call ath9k_tx99_deinit().
sc->tx99_state would be freed. After that, the first thread
would continue inside ath9k_tx99_init() and call
r = ath9k_tx99_send(sc, sc->tx99_skb, &txctl);
that would make use of the freed sc->tx99_skb memory.

Signed-off-by: Miaoqing Pan <miaoqing@codeaurora.org>
---
 drivers/net/wireless/ath/ath9k/tx99.c | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/drivers/net/wireless/ath/ath9k/tx99.c b/drivers/net/wireless/ath/ath9k/tx99.c
index a866cbd..49ed1af 100644
--- a/drivers/net/wireless/ath/ath9k/tx99.c
+++ b/drivers/net/wireless/ath/ath9k/tx99.c
@@ -189,22 +189,27 @@ static ssize_t write_file_tx99(struct file *file, const char __user *user_buf,
 	if (strtobool(buf, &start))
 		return -EINVAL;
 
+	mutex_lock(&sc->mutex);
+
 	if (start == sc->tx99_state) {
 		if (!start)
-			return count;
+			goto out;
 		ath_dbg(common, XMIT, "Resetting TX99\n");
 		ath9k_tx99_deinit(sc);
 	}
 
 	if (!start) {
 		ath9k_tx99_deinit(sc);
-		return count;
+		goto out;
 	}
 
 	r = ath9k_tx99_init(sc);
-	if (r)
+	if (r) {
+		mutex_unlock(&sc->mutex);
 		return r;
-
+	}
+out:
+	mutex_unlock(&sc->mutex);
 	return count;
 }
 
-- 
1.9.1

[PATCH 2/2] ath9k: fix tx99 bus error

[miaoqing]

From: Miaoqing Pan <miaoqing@codeaurora.org>

The hard coded register 0x9864 and 0x9924 are invalid
for ar9300 chips.

Signed-off-by: Miaoqing Pan <miaoqing@codeaurora.org>
---
 drivers/net/wireless/ath/ath9k/ar9003_phy.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/net/wireless/ath/ath9k/ar9003_phy.c b/drivers/net/wireless/ath/ath9k/ar9003_phy.c
index ae304355..fe5102c 100644
--- a/drivers/net/wireless/ath/ath9k/ar9003_phy.c
+++ b/drivers/net/wireless/ath/ath9k/ar9003_phy.c
@@ -1821,8 +1821,6 @@ static void ar9003_hw_spectral_scan_wait(struct ath_hw *ah)
 static void ar9003_hw_tx99_start(struct ath_hw *ah, u32 qnum)
 {
 	REG_SET_BIT(ah, AR_PHY_TEST, PHY_AGC_CLR);
-	REG_SET_BIT(ah, 0x9864, 0x7f000);
-	REG_SET_BIT(ah, 0x9924, 0x7f00fe);
 	REG_CLR_BIT(ah, AR_DIAG_SW, AR_DIAG_RX_DIS);
 	REG_WRITE(ah, AR_CR, AR_CR_RXD);
 	REG_WRITE(ah, AR_DLCL_IFS(qnum), 0);
-- 
1.9.1

Re: [PATCH 2/2] ath9k: fix tx99 bus error

[Sven Eckelmann]

[-- Attachment #1: Type: text/plain, Size: 1656 bytes --]

On Dienstag, 20. Juni 2017 09:13:40 CEST miaoqing@codeaurora.org wrote:
> From: Miaoqing Pan <miaoqing@codeaurora.org>
> 
> The hard coded register 0x9864 and 0x9924 are invalid
> for ar9300 chips.
[...]
> -	REG_SET_BIT(ah, 0x9864, 0x7f000);
> -	REG_SET_BIT(ah, 0x9924, 0x7f00fe);

Sorry that this messages comes so later after the patch was accepted. But what 
were these things expected to do? My guess is that 0x9864 is AR_PHY_CCA and 
the other one is something else (AR_PHY_TIMING5?). But yes, these would be ar9002
and not AR9003.

What are the problems that we would expect when the CCA threshold and the CYCPWR 
threshold are no longer be set to the highest possible value? Are we now expecting 
that the device is not transmitting at 99% when it sees other signals?

Btw. why are you writing that ar9300 chips don't have this? It looks to me 
like the original code was taken from QCA's 9300 code [1]. Was it always broken
in the AR9300 hal and how was it now fixed with with the newer HALs?

Could it be that newer chips just have it mapped to a different location? AGC 
on the 9300 seems to be at 0x9e00 and maybe the cca register should have been 
0x9e1c (AR_PHY_CCA_0) and should be set to AR_PHY_CCA_THRESH62? And there is 
also a AR_PHY_TIMING5 (0x9810) which might have to be set to 
AR_PHY_TIMING5_CYCPWR_THR1 | AR_PHY_TIMING5_CYCPWR_THR1A. Of course, I have 
absolutely no idea whether these registers actually control the 
same thing and whether the settings are correct.

Kind regards,
	Sven

[1] https://github.com/freebsd/freebsd/blob/386ddae58459341ec567604707805814a2128a57/sys/contrib/dev/ath/ath_hal/ar9300/ar9300_tx99_tgt.c#L502

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [1/2] ath9k: fix tx99 use after free

[Kalle Valo]

miaoqing pan <miaoqing@codeaurora.org> wrote:

> One scenario that could lead to UAF is two threads writing
> simultaneously to the "tx99" debug file. One of them would
> set the "start" value to true and follow to ath9k_tx99_init().
> Inside the function it would set the sc->tx99_state to true
> after allocating sc->tx99skb. Then, the other thread would
> execute write_file_tx99() and call ath9k_tx99_deinit().
> sc->tx99_state would be freed. After that, the first thread
> would continue inside ath9k_tx99_init() and call
> r = ath9k_tx99_send(sc, sc->tx99_skb, &txctl);
> that would make use of the freed sc->tx99_skb memory.
> 
> Signed-off-by: Miaoqing Pan <miaoqing@codeaurora.org>
> Signed-off-by: Kalle Valo <kvalo@qca.qualcomm.com>

I added Cc stable to both patches.

-- 
https://patchwork.kernel.org/patch/9798309/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

Re: [1/2] ath9k: fix tx99 use after free

[Kalle Valo]

miaoqing pan <miaoqing@codeaurora.org> wrote:

> One scenario that could lead to UAF is two threads writing
> simultaneously to the "tx99" debug file. One of them would
> set the "start" value to true and follow to ath9k_tx99_init().
> Inside the function it would set the sc->tx99_state to true
> after allocating sc->tx99skb. Then, the other thread would
> execute write_file_tx99() and call ath9k_tx99_deinit().
> sc->tx99_state would be freed. After that, the first thread
> would continue inside ath9k_tx99_init() and call
> r = ath9k_tx99_send(sc, sc->tx99_skb, &txctl);
> that would make use of the freed sc->tx99_skb memory.
> 
> Cc: <stable@vger.kernel.org>
> Signed-off-by: Miaoqing Pan <miaoqing@codeaurora.org>
> Signed-off-by: Kalle Valo <kvalo@qca.qualcomm.com>

2 patches applied to ath-next branch of ath.git, thanks.

cf8ce1ea61b7 ath9k: fix tx99 use after free
bde717ab4736 ath9k: fix tx99 bus error

-- 
https://patchwork.kernel.org/patch/9798309/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches