ath9k_htc: fix potential out of bounds access with invalid rxstatus->rs_keyix

PATCH In review Dan Carpenter 4 participants 8 messages Apr 9, 2022 – Apr 23, 2022

Subsystems (2)

Kernel janitors Qualcomm Atheros ATH9K wireless driver

lore

Share & tools

lore

Dan Carpenter

author

2022-04-09 06:13 UTC #0 lore raw

PATCH 1/1 ath9k_htc: fix potential out of bounds access with invalid rxstatus->rs_keyix

The "rxstatus->rs_keyix" eventually gets passed to test_bit() so we need to ensure that it is within than bitmap.

drivers/net/wireless/ath/ath9k/common.c:46 ath9k_cmn_rx_accept()
error: passing untrusted data 'rx_stats->rs_keyix' to 'test_bit()'

Fixes 4ed1a8d4a257 ("ath9k_htc: use ath9k_cmn_rx_accept")

Signed-off-by Dan Carpenter <[email protected]>

---
 drivers/net/wireless/ath/ath9k/htc_drv_txrx.c | 8 ++++++++
 1 file changed, 8 insertions(+)

drivers/net/wireless/ath/ath9k/htc_drv_txrx.c +8 -0

index 6a850a0bfa8a..a23eaca0326d 100644
@@ -1016,6 +1016,14 @@ static bool ath9k_rx_prepare(struct ath9k_htc_priv *priv,
 		goto rx_next;
 	}
 
+	if (rxstatus->rs_keyix >= ATH_KEYMAX &&
+	    rxstatus->rs_keyix != ATH9K_RXKEYIX_INVALID) {
+		ath_dbg(common, ANY,
+			"Invalid keyix, dropping (keyix: %d)\n",
+			rxstatus->rs_keyix);
+		goto rx_next;
+	}
+
 	/* Get the RX status information */
 
 	memset(rx_status, 0, sizeof(struct ieee80211_rx_status));

Signature · 1 line

2.20.1

Oleksij Rempel

2022-04-09 07:54 UTC #1 lore raw

Hi Dan,

thank you for your patch.

Am 09.04.22 um 08:12 schrieb Dan Carpenter:

Quoted 21 lines

The "rxstatus->rs_keyix" eventually gets passed to test_bit() so we need to ensure that it is within than bitmap.
drivers/net/wireless/ath/ath9k/common.c:46 ath9k_cmn_rx_accept()
error: passing untrusted data 'rx_stats->rs_keyix' to 'test_bit()'
Fixes 4ed1a8d4a257 ("ath9k_htc: use ath9k_cmn_rx_accept")
Signed-off-by Dan Carpenter <[email protected]>
---
  drivers/net/wireless/ath/ath9k/htc_drv_txrx.c | 8 ++++++++
  1 file changed, 8 insertions(+)
drivers/net/wireless/ath/ath9k/htc_drv_txrx.c +1 -0
index 6a850a0bfa8a..a23eaca0326d 100644
@@ -1016,6 +1016,14 @@ static bool ath9k_rx_prepare(struct ath9k_htc_priv *priv,
  		goto rx_next;
  	}
 
+	if (rxstatus->rs_keyix >= ATH_KEYMAX &&

drivers/net/wireless/ath/ath9k/htc_drv_txrx.c static bool ath9k_rx_prepare(struct ath9k_htc_priv *priv, · line 1029

Quoted 10 lines

drivers/net/wireless/ath/ath9k/htc_drv_txrx.c +7 -0

+	    rxstatus->rs_keyix != ATH9K_RXKEYIX_INVALID) {
+		ath_dbg(common, ANY,
+			"Invalid keyix, dropping (keyix: %d)\n",
+			rxstatus->rs_keyix);
+		goto rx_next;
+	}
+
  	/* Get the RX status information */
 
  	memset(rx_status, 0, sizeof(struct ieee80211_rx_status));

Looks ok to me.

By the way, rs_keyix seems to have a wrong type. It is declared as u8, but used as s8.

--
Regards,
Oleksij

Dan Carpenter

author

2022-04-11 07:55 UTC #2 lore raw

Quoted 18 lines · Oleksij Rempel

Quoted 18 lines

drivers/net/wireless/ath/ath9k/htc_drv_txrx.c +8 -0

index 6a850a0bfa8a..a23eaca0326d 100644
@@ -1016,6 +1016,14 @@ static bool ath9k_rx_prepare(struct ath9k_htc_priv *priv,
  		goto rx_next;
  	}
 
+	if (rxstatus->rs_keyix >= ATH_KEYMAX &&
+	    rxstatus->rs_keyix != ATH9K_RXKEYIX_INVALID) {
+		ath_dbg(common, ANY,
+			"Invalid keyix, dropping (keyix: %d)\n",
+			rxstatus->rs_keyix);
+		goto rx_next;
+	}
+
  	/* Get the RX status information */

Quoted 3 lines

drivers/net/wireless/ath/ath9k/htc_drv_txrx.c static bool ath9k_rx_prepare(struct ath9k_htc_priv *priv, · line 1029
Quoted 1 line
drivers/net/wireless/ath/ath9k/htc_drv_txrx.c +0 -0
  	memset(rx_status, 0, sizeof(struct ieee80211_rx_status));
Looks ok to me.

Thanks!

Quoted 2 lines

By the way, rs_keyix seems to have a wrong type. It is declared as u8, but used as s8.

That sounds like something outside the scope of the patch... Why do you mean "used as s8"? Which function are you talking about?

You made me panic briefly because ATH9K_RXKEYIX_INVALID is a u8 so it needs to be u8. I would have thought instinctively that u8 would be the right type for an index like this.

regards,
dan carpenter

Oleksij Rempel

2022-04-11 10:26 UTC #3 lore raw

Am 11.04.22 um 09:54 schrieb Dan Carpenter:

Quoted 23 lines · Oleksij Rempel

Quoted 18 lines
Quoted 18 lines
drivers/net/wireless/ath/ath9k/htc_drv_txrx.c +8 -0
index 6a850a0bfa8a..a23eaca0326d 100644
@@ -1016,6 +1016,14 @@ static bool ath9k_rx_prepare(struct ath9k_htc_priv *priv,
   		goto rx_next;
   	}
 
+	if (rxstatus->rs_keyix >= ATH_KEYMAX &&
+	    rxstatus->rs_keyix != ATH9K_RXKEYIX_INVALID) {
+		ath_dbg(common, ANY,
+			"Invalid keyix, dropping (keyix: %d)\n",
+			rxstatus->rs_keyix);
+		goto rx_next;
+	}
+
   	/* Get the RX status information */
 
Quoted 3 lines
drivers/net/wireless/ath/ath9k/htc_drv_txrx.c static bool ath9k_rx_prepare(struct ath9k_htc_priv *priv, · line 1029
Quoted 1 line
drivers/net/wireless/ath/ath9k/htc_drv_txrx.c +0 -0
   	memset(rx_status, 0, sizeof(struct ieee80211_rx_status));
Looks ok to me.

Quoted 6 lines

Thanks!
Quoted 2 lines
By the way, rs_keyix seems to have a wrong type. It is declared as u8, but used as s8.
That sounds like something outside the scope of the patch...

ack :)

Quoted 5 lines

Why do you mean "used as s8"? Which function are you talking about?
You made me panic briefly because ATH9K_RXKEYIX_INVALID is a u8 so it needs to be u8. I would have thought instinctively that u8 would be the right type for an index like this.

Because ATH_KEYMAX == S8_MAX and ATH9K_RXKEYIX_INVALID is ((u8)-1)

All bitmap values within drivers/net/wireless/ath should never have BIT(7) set, except it is -1.

--
Regards,
Oleksij

Toke Høiland-Jørgensen

maintainer Acked-by on v1 1/1

2022-04-09 21:37 UTC #4 lore raw

In reply to Dan Carpenter

Quoted 2 lines · Dan Carpenter

The "rxstatus->rs_keyix" eventually gets passed to test_bit() so we need to ensure that it is within than bitmap.

s/than/the/ ?

This I think Kalle can fix up when applying :)

Quoted 5 lines

drivers/net/wireless/ath/ath9k/common.c:46 ath9k_cmn_rx_accept()
error: passing untrusted data 'rx_stats->rs_keyix' to 'test_bit()'
Fixes 4ed1a8d4a257 ("ath9k_htc: use ath9k_cmn_rx_accept")
Signed-off-by Dan Carpenter <[email protected]>

Acked-by Toke Høiland-Jørgensen <[email protected]>

Kalle Valo

2022-04-12 13:27 UTC #5 lore raw

Quoted 8 lines · Toke Høiland-Jørgensen

Quoted 2 lines · Dan Carpenter
The "rxstatus->rs_keyix" eventually gets passed to test_bit() so we need to ensure that it is within than bitmap.
s/than/the/ ?
This I think Kalle can fix up when applying :)

Yup, fixed now in the pending branch.

Signature · 3 lines

https://patchwork.kernel.org/project/linux-wireless/list/

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

Dan Carpenter

author

2022-04-12 13:30 UTC #6 lore raw

Quoted 4 lines · Kalle Valo

Quoted 2 lines · Toke Høiland-Jørgensen
Dan Carpenter <[email protected]> writes:

Quoted 8 lines

Quoted 6 lines
Quoted 2 lines
The "rxstatus->rs_keyix" eventually gets passed to test_bit() so we need to ensure that it is within than bitmap.
s/than/the/ ?
This I think Kalle can fix up when applying :)
Yup, fixed now in the pending branch.

Thanks!

regards,
dan carpenter

Kalle Valo

2022-04-23 09:32 UTC #7 lore raw

In reply to Dan Carpenter

Quoted 10 lines · Dan Carpenter

The "rxstatus->rs_keyix" eventually gets passed to test_bit() so we need to ensure that it is within the bitmap.
drivers/net/wireless/ath/ath9k/common.c:46 ath9k_cmn_rx_accept()
error: passing untrusted data 'rx_stats->rs_keyix' to 'test_bit()'
Fixes 4ed1a8d4a257 ("ath9k_htc: use ath9k_cmn_rx_accept")
Signed-off-by Dan Carpenter <[email protected]>
Acked-by Toke Høiland-Jørgensen <[email protected]>
Signed-off-by Kalle Valo <[email protected]>

Patch applied to ath-next branch of ath.git, thanks.

2dc509305cf9 ath9k_htc: fix potential out of bounds access with invalid rxstatus->rs_keyix

Signature · 3 lines

https://patchwork.kernel.org/project/linux-wireless/patch/20220409061225.GA5447@kili/

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