From: Miaoqing Pan <[email protected]>
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 <[email protected]>
---
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
miaoqing pan <[email protected]> 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: <[email protected]>
> Signed-off-by: Miaoqing Pan <[email protected]>
> Signed-off-by: Kalle Valo <[email protected]>
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
miaoqing pan <[email protected]> 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 <[email protected]>
> Signed-off-by: Kalle Valo <[email protected]>
I added Cc stable to both patches.
--
https://patchwork.kernel.org/patch/9798309/
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
From: Miaoqing Pan <[email protected]>
The hard coded register 0x9864 and 0x9924 are invalid
for ar9300 chips.
Signed-off-by: Miaoqing Pan <[email protected]>
---
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
On Dienstag, 20. Juni 2017 09:13:40 CEST [email protected] wrote:
> From: Miaoqing Pan <[email protected]>
>
> 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