Headline
CVE-2022-1679: [PATCH v3 1/2] ath9k: fix use-after-free in ath9k_hif_usb_rx_cb
A use-after-free flaw was found in the Linux kernel’s Atheros wireless adapter driver in the way a user forces the ath9k_htc_wait_for_target function to fail with some input messages. This flaw allows a local user to crash or potentially escalate their privileges on the system.
* [PATCH v3 1/2] ath9k: fix use-after-free in ath9k_hif_usb_rx_cb @ 2022-02-07 20:24 Pavel Skripkin 2022-02-07 20:24 ` [PATCH v3 2/2] ath9k: htc: clean up *STAT_* macros Pavel Skripkin ` (3 more replies) 0 siblings, 4 replies; 19+ messages in thread From: Pavel Skripkin @ 2022-02-07 20:24 UTC (permalink / raw) To: ath9k-devel, kvalo, davem, kuba, toke, linville Cc: linux-wireless, netdev, linux-kernel, Pavel Skripkin, syzbot+03110230a11411024147, syzbot+c6dde1f690b60e0b9fbe
Syzbot reported use-after-free Read in ath9k_hif_usb_rx_cb(). The problem was in incorrect htc_handle->drv_priv initialization.
Probable call trace which can trigger use-after-free:
ath9k_htc_probe_device() /* htc_handle->drv_priv = priv; */ ath9k_htc_wait_for_target() <— Failed ieee80211_free_hw() <— priv pointer is freed
<IRQ> … ath9k_hif_usb_rx_cb() ath9k_hif_usb_rx_stream() RX_STAT_INC() <— htc_handle->drv_priv access
In order to not add fancy protection for drv_priv we can move htc_handle->drv_priv initialization at the end of the ath9k_htc_probe_device() and add helper macro to make all *_STAT_* macros NULL save.
Fixes: fb9987d0f748 (“ath9k_htc: Support for AR9271 chipset.”) Reported-and-tested-by: [email protected] Reported-and-tested-by: [email protected] Signed-off-by: Pavel Skripkin [email protected]
Changes since v2: - My send-email script forgot, that mailing lists exist. Added back all related lists
Changes since v1: - Removed clean-ups and moved them to 2/2
drivers/net/wireless/ath/ath9k/htc.h | 10 ++++±---- drivers/net/wireless/ath/ath9k/htc_drv_init.c | 3 +± 2 files changed, 7 insertions(+), 6 deletions(-)
diff --git a/drivers/net/wireless/ath/ath9k/htc.h b/drivers/net/wireless/ath/ath9k/htc.h index 6b45e63fae4b…141642e5e00d 100644 — a/drivers/net/wireless/ath/ath9k/htc.h +++ b/drivers/net/wireless/ath/ath9k/htc.h @@ -327,11 +327,11 @@ static inline struct ath9k_htc_tx_ctl *HTC_SKB_CB(struct sk_buff *skb) }
#ifdef CONFIG_ATH9K_HTC_DEBUGFS - -#define TX_STAT_INC© (hif_dev->htc_handle->drv_priv->debug.tx_stats.c++) -#define TX_STAT_ADD(c, a) (hif_dev->htc_handle->drv_priv->debug.tx_stats.c += a) -#define RX_STAT_INC© (hif_dev->htc_handle->drv_priv->debug.skbrx_stats.c++) -#define RX_STAT_ADD(c, a) (hif_dev->htc_handle->drv_priv->debug.skbrx_stats.c += a) +#define __STAT_SAVE(expr) (hif_dev->htc_handle->drv_priv ? (expr) : 0) +#define TX_STAT_INC© __STAT_SAVE(hif_dev->htc_handle->drv_priv->debug.tx_stats.c++) +#define TX_STAT_ADD(c, a) __STAT_SAVE(hif_dev->htc_handle->drv_priv->debug.tx_stats.c += a) +#define RX_STAT_INC© __STAT_SAVE(hif_dev->htc_handle->drv_priv->debug.skbrx_stats.c++) +#define RX_STAT_ADD(c, a) __STAT_SAVE(hif_dev->htc_handle->drv_priv->debug.skbrx_stats.c += a) #define CAB_STAT_INC priv->debug.tx_stats.cab_queued++
#define TX_QSTAT_INC(q) (priv->debug.tx_stats.queue_stats[q]++) diff --git a/drivers/net/wireless/ath/ath9k/htc_drv_init.c b/drivers/net/wireless/ath/ath9k/htc_drv_init.c index ff61ae34ecdf…07ac88fb1c57 100644 — a/drivers/net/wireless/ath/ath9k/htc_drv_init.c +++ b/drivers/net/wireless/ath/ath9k/htc_drv_init.c @@ -944,7 +944,6 @@ int ath9k_htc_probe_device(struct htc_target *htc_handle, struct device *dev, priv->hw = hw; priv->htc = htc_handle; priv->dev = dev; - htc_handle->drv_priv = priv; SET_IEEE80211_DEV(hw, priv->dev);
ret = ath9k\_htc\_wait\_for\_target(priv);
@@ -965,6 +964,8 @@ int ath9k_htc_probe_device(struct htc_target *htc_handle, struct device *dev, if (ret) goto err_init;
- htc_handle->drv_priv = priv;
- return 0;
err_init:
2.34.1
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH v3 2/2] ath9k: htc: clean up *STAT_* macros 2022-02-07 20:24 [PATCH v3 1/2] ath9k: fix use-after-free in ath9k_hif_usb_rx_cb Pavel Skripkin @ 2022-02-07 20:24 ` Pavel Skripkin 2022-02-07 21:24 ` Jeff Johnson 2022-02-08 14:47 ` [PATCH v3 1/2] ath9k: fix use-after-free in ath9k_hif_usb_rx_cb Toke Høiland-Jørgensen ` (2 subsequent siblings) 3 siblings, 1 reply; 19+ messages in thread From: Pavel Skripkin @ 2022-02-07 20:24 UTC (permalink / raw) To: ath9k-devel, kvalo, davem, kuba, toke, linville Cc: linux-wireless, netdev, linux-kernel, Pavel Skripkin
I’ve changed *STAT_* macros a bit in previous patch and I seems like they become really unreadable. Align these macros definitions to make code cleaner.
Also fixed following checkpatch warning
ERROR: Macros with complex values should be enclosed in parentheses
Signed-off-by: Pavel Skripkin [email protected]
Changes since v2: - My send-email script forgot, that mailing lists exist. Added back all related lists - Fixed checkpatch warning
Changes since v1: - Added this patch
drivers/net/wireless/ath/ath9k/htc.h | 16 +++++++±------- 1 file changed, 8 insertions(+), 8 deletions(-)
diff --git a/drivers/net/wireless/ath/ath9k/htc.h b/drivers/net/wireless/ath/ath9k/htc.h index 141642e5e00d…b4755e21a501 100644 — a/drivers/net/wireless/ath/ath9k/htc.h +++ b/drivers/net/wireless/ath/ath9k/htc.h @@ -327,14 +327,14 @@ static inline struct ath9k_htc_tx_ctl *HTC_SKB_CB(struct sk_buff *skb) }
#ifdef CONFIG_ATH9K_HTC_DEBUGFS -#define __STAT_SAVE(expr) (hif_dev->htc_handle->drv_priv ? (expr) : 0) -#define TX_STAT_INC© __STAT_SAVE(hif_dev->htc_handle->drv_priv->debug.tx_stats.c++) -#define TX_STAT_ADD(c, a) __STAT_SAVE(hif_dev->htc_handle->drv_priv->debug.tx_stats.c += a) -#define RX_STAT_INC© __STAT_SAVE(hif_dev->htc_handle->drv_priv->debug.skbrx_stats.c++) -#define RX_STAT_ADD(c, a) __STAT_SAVE(hif_dev->htc_handle->drv_priv->debug.skbrx_stats.c += a) -#define CAB_STAT_INC priv->debug.tx_stats.cab_queued++
-#define TX_QSTAT_INC(q) (priv->debug.tx_stats.queue_stats[q]++) +#define __STAT_SAVE(expr) (hif_dev->htc_handle->drv_priv ? (expr) : 0) +#define TX_STAT_INC© __STAT_SAVE(hif_dev->htc_handle->drv_priv->debug.tx_stats.c++) +#define TX_STAT_ADD(c, a) __STAT_SAVE(hif_dev->htc_handle->drv_priv->debug.tx_stats.c += a) +#define RX_STAT_INC© __STAT_SAVE(hif_dev->htc_handle->drv_priv->debug.skbrx_stats.c++) +#define RX_STAT_ADD(c, a) __STAT_SAVE(hif_dev->htc_handle->drv_priv->debug.skbrx_stats.c += a) +#define CAB_STAT_INC (priv->debug.tx_stats.cab_queued++)
+#define TX_QSTAT_INC(q) (priv->debug.tx_stats.queue_stats[q]++)
void ath9k_htc_err_stat_rx(struct ath9k_htc_priv *priv, struct ath_rx_status *rs); – 2.34.1
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v3 2/2] ath9k: htc: clean up *STAT_* macros 2022-02-07 20:24 ` [PATCH v3 2/2] ath9k: htc: clean up *STAT_* macros Pavel Skripkin @ 2022-02-07 21:24 ` Jeff Johnson 2022-02-08 15:32 ` Toke Høiland-Jørgensen 0 siblings, 1 reply; 19+ messages in thread From: Jeff Johnson @ 2022-02-07 21:24 UTC (permalink / raw) To: Pavel Skripkin, ath9k-devel, kvalo, davem, kuba, toke, linville Cc: linux-wireless, netdev, linux-kernel
On 2/7/2022 12:24 PM, Pavel Skripkin wrote: > I’ve changed *STAT_* macros a bit in previous patch and I seems like
they become really unreadable. Align these macros definitions to make code cleaner.
Also fixed following checkpatch warning
ERROR: Macros with complex values should be enclosed in parentheses
Signed-off-by: Pavel Skripkin [email protected]
Changes since v2:
- My send-email script forgot, that mailing lists exist. Added back all related lists
- Fixed checkpatch warning
Changes since v1:
- Added this patch
drivers/net/wireless/ath/ath9k/htc.h | 16 +++++++±------- 1 file changed, 8 insertions(+), 8 deletions(-)
diff --git a/drivers/net/wireless/ath/ath9k/htc.h b/drivers/net/wireless/ath/ath9k/htc.h index 141642e5e00d…b4755e21a501 100644 — a/drivers/net/wireless/ath/ath9k/htc.h +++ b/drivers/net/wireless/ath/ath9k/htc.h @@ -327,14 +327,14 @@ static inline struct ath9k_htc_tx_ctl *HTC_SKB_CB(struct sk_buff *skb) }
#ifdef CONFIG_ATH9K_HTC_DEBUGFS -#define __STAT_SAVE(expr) (hif_dev->htc_handle->drv_priv ? (expr) : 0) -#define TX_STAT_INC© __STAT_SAVE(hif_dev->htc_handle->drv_priv->debug.tx_stats.c++) -#define TX_STAT_ADD(c, a) __STAT_SAVE(hif_dev->htc_handle->drv_priv->debug.tx_stats.c += a) -#define RX_STAT_INC© __STAT_SAVE(hif_dev->htc_handle->drv_priv->debug.skbrx_stats.c++) -#define RX_STAT_ADD(c, a) __STAT_SAVE(hif_dev->htc_handle->drv_priv->debug.skbrx_stats.c += a) -#define CAB_STAT_INC priv->debug.tx_stats.cab_queued++
-#define TX_QSTAT_INC(q) (priv->debug.tx_stats.queue_stats[q]++) +#define __STAT_SAVE(expr) (hif_dev->htc_handle->drv_priv ? (expr) : 0) +#define TX_STAT_INC© __STAT_SAVE(hif_dev->htc_handle->drv_priv->debug.tx_stats.c++) +#define TX_STAT_ADD(c, a) __STAT_SAVE(hif_dev->htc_handle->drv_priv->debug.tx_stats.c += a) +#define RX_STAT_INC© __STAT_SAVE(hif_dev->htc_handle->drv_priv->debug.skbrx_stats.c++) +#define RX_STAT_ADD(c, a) __STAT_SAVE(hif_dev->htc_handle->drv_priv->debug.skbrx_stats.c += a) +#define CAB_STAT_INC (priv->debug.tx_stats.cab_queued++)
+#define TX_QSTAT_INC(q) (priv->debug.tx_stats.queue_stats[q]++)
void ath9k_htc_err_stat_rx(struct ath9k_htc_priv *priv, struct ath_rx_status *rs); It seems that these macros (both the original and the new) aren’t following the guidance from the Coding Style which tells us under “Things to avoid when using macros” that we should avoid "macros that depend on having a local variable with a magic name". Wouldn’t these macros be “better” is they included the hif_dev/priv as arguments rather than being "magic"?
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v3 2/2] ath9k: htc: clean up *STAT_* macros 2022-02-07 21:24 ` Jeff Johnson @ 2022-02-08 15:32 ` Toke Høiland-Jørgensen 2022-02-08 15:49 ` Pavel Skripkin 0 siblings, 1 reply; 19+ messages in thread From: Toke Høiland-Jørgensen @ 2022-02-08 15:32 UTC (permalink / raw) To: Jeff Johnson, Pavel Skripkin, ath9k-devel, kvalo, davem, kuba, linville Cc: linux-wireless, netdev, linux-kernel
Jeff Johnson <[email protected]> writes:
> On 2/7/2022 12:24 PM, Pavel Skripkin wrote:
I’ve changed *STAT_* macros a bit in previous patch and I seems like they become really unreadable. Align these macros definitions to make code cleaner.
Also fixed following checkpatch warning
ERROR: Macros with complex values should be enclosed in parentheses
Signed-off-by: Pavel Skripkin [email protected]
Changes since v2:
- My send-email script forgot, that mailing lists exist. Added back all related lists
- Fixed checkpatch warning
Changes since v1:
- Added this patch
drivers/net/wireless/ath/ath9k/htc.h | 16 +++++++±------- 1 file changed, 8 insertions(+), 8 deletions(-)
diff --git a/drivers/net/wireless/ath/ath9k/htc.h b/drivers/net/wireless/ath/ath9k/htc.h index 141642e5e00d…b4755e21a501 100644 — a/drivers/net/wireless/ath/ath9k/htc.h +++ b/drivers/net/wireless/ath/ath9k/htc.h @@ -327,14 +327,14 @@ static inline struct ath9k_htc_tx_ctl *HTC_SKB_CB(struct sk_buff *skb) }
#ifdef CONFIG_ATH9K_HTC_DEBUGFS -#define __STAT_SAVE(expr) (hif_dev->htc_handle->drv_priv ? (expr) : 0) -#define TX_STAT_INC© __STAT_SAVE(hif_dev->htc_handle->drv_priv->debug.tx_stats.c++) -#define TX_STAT_ADD(c, a) __STAT_SAVE(hif_dev->htc_handle->drv_priv->debug.tx_stats.c += a) -#define RX_STAT_INC© __STAT_SAVE(hif_dev->htc_handle->drv_priv->debug.skbrx_stats.c++) -#define RX_STAT_ADD(c, a) __STAT_SAVE(hif_dev->htc_handle->drv_priv->debug.skbrx_stats.c += a) -#define CAB_STAT_INC priv->debug.tx_stats.cab_queued++
-#define TX_QSTAT_INC(q) (priv->debug.tx_stats.queue_stats[q]++) +#define __STAT_SAVE(expr) (hif_dev->htc_handle->drv_priv ? (expr) : 0) +#define TX_STAT_INC© __STAT_SAVE(hif_dev->htc_handle->drv_priv->debug.tx_stats.c++) +#define TX_STAT_ADD(c, a) __STAT_SAVE(hif_dev->htc_handle->drv_priv->debug.tx_stats.c += a) +#define RX_STAT_INC© __STAT_SAVE(hif_dev->htc_handle->drv_priv->debug.skbrx_stats.c++) +#define RX_STAT_ADD(c, a) __STAT_SAVE(hif_dev->htc_handle->drv_priv->debug.skbrx_stats.c += a) +#define CAB_STAT_INC (priv->debug.tx_stats.cab_queued++)
+#define TX_QSTAT_INC(q) (priv->debug.tx_stats.queue_stats[q]++)
void ath9k_htc_err_stat_rx(struct ath9k_htc_priv *priv, struct ath_rx_status *rs);
It seems that these macros (both the original and the new) aren’t following the guidance from the Coding Style which tells us under “Things to avoid when using macros” that we should avoid "macros that depend on having a local variable with a magic name". Wouldn’t these macros be “better” is they included the hif_dev/priv as arguments rather than being "magic"? Hmm, yeah, that’s a good point; looks like the non-HTC ath9k stats macros have already been converted to take the container as a parameter, so taking this opportunity to fix these macros is not a bad idea. While we’re at it, let’s switch to the do{} while(0) syntax the other macros are using instead of that weird usage of ?:. And there’s not really any reason for the duplication between ADD/INC either. So I’m thinking something like:
#define __STAT_SAVE(_priv, _member, _n) do { if (_priv) (_priv)->_member += (_n); } while(0)
#define TX_STAT_ADD(_priv, _c, _a) __STAT_SAVE(_priv, debug.tx_stats._c, _a) #define TX_STAT_INC(_priv, _c) TX_STAT_ADD(_priv, _c, 1)
[… etc …]
-Toke
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v3 2/2] ath9k: htc: clean up *STAT_* macros 2022-02-08 15:32 ` Toke Høiland-Jørgensen @ 2022-02-08 15:49 ` Pavel Skripkin 0 siblings, 0 replies; 19+ messages in thread From: Pavel Skripkin @ 2022-02-08 15:49 UTC (permalink / raw) To: Toke Høiland-Jørgensen, Jeff Johnson, ath9k-devel, kvalo, davem, kuba, linville Cc: linux-wireless, netdev, linux-kernel
Hi Toke,
On 2/8/22 18:32, Toke Høiland-Jørgensen wrote: >> It seems that these macros (both the original and the new) aren’t
following the guidance from the Coding Style which tells us under “Things to avoid when using macros” that we should avoid "macros that depend on having a local variable with a magic name". Wouldn’t these macros be “better” is they included the hif_dev/priv as arguments rather than being "magic"?
Hmm, yeah, that’s a good point; looks like the non-HTC ath9k stats macros have already been converted to take the container as a parameter, so taking this opportunity to fix these macros is not a bad idea. While we’re at it, let’s switch to the do{} while(0) syntax the other macros are using instead of that weird usage of ?:. And there’s not really any reason for the duplication between ADD/INC either. So I’m thinking something like:
#define __STAT_SAVE(_priv, _member, _n) do { if (_priv) (_priv)->_member += (_n); } while(0)
#define TX_STAT_ADD(_priv, _c, _a) __STAT_SAVE(_priv, debug.tx_stats._c, _a) #define TX_STAT_INC(_priv, _c) TX_STAT_ADD(_priv, _c, 1)
Good point, thank you. Will redo these macros in v4
With regards,
Pavel Skripkin
^ permalink raw reply \[flat|**nested**\] 19+ messages in thread
* Re: [PATCH v3 1/2] ath9k: fix use-after-free in ath9k_hif_usb_rx_cb 2022-02-07 20:24 [PATCH v3 1/2] ath9k: fix use-after-free in ath9k_hif_usb_rx_cb Pavel Skripkin 2022-02-07 20:24 ` [PATCH v3 2/2] ath9k: htc: clean up *STAT_* macros Pavel Skripkin @ 2022-02-08 14:47 ` Toke Høiland-Jørgensen 2022-02-08 15:48 ` Pavel Skripkin 2022-05-12 12:49 ` Toke Høiland-Jørgensen 2022-05-12 16:05 ` Jeff Johnson 3 siblings, 1 reply; 19+ messages in thread From: Toke Høiland-Jørgensen @ 2022-02-08 14:47 UTC (permalink / raw) To: Pavel Skripkin, ath9k-devel, kvalo, davem, kuba, linville Cc: linux-wireless, netdev, linux-kernel, Pavel Skripkin, syzbot+03110230a11411024147, syzbot+c6dde1f690b60e0b9fbe
Pavel Skripkin [email protected] writes:
> Syzbot reported use-after-free Read in ath9k_hif_usb_rx_cb(). The
problem was in incorrect htc_handle->drv_priv initialization.
Probable call trace which can trigger use-after-free:
ath9k_htc_probe_device() /* htc_handle->drv_priv = priv; */ ath9k_htc_wait_for_target() <— Failed ieee80211_free_hw() <— priv pointer is freed
<IRQ> … ath9k_hif_usb_rx_cb() ath9k_hif_usb_rx_stream() RX_STAT_INC() <— htc_handle->drv_priv access
In order to not add fancy protection for drv_priv we can move htc_handle->drv_priv initialization at the end of the ath9k_htc_probe_device() and add helper macro to make all *_STAT_* macros NULL save. I’m not too familiar with how the initialisation flow of an ath9k_htc device works. Looking at htc_handle->drv_priv there seems to be three other functions apart from the stat counters that dereference it:
ath9k_htc_suspend() ath9k_htc_resume() ath9k_hif_usb_disconnect()
What guarantees that none of these will be called midway through ath9k_htc_probe_device() (which would lead to a NULL deref after this change)?
-Toke
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v3 1/2] ath9k: fix use-after-free in ath9k_hif_usb_rx_cb 2022-02-08 14:47 ` [PATCH v3 1/2] ath9k: fix use-after-free in ath9k_hif_usb_rx_cb Toke Høiland-Jørgensen @ 2022-02-08 15:48 ` Pavel Skripkin 2022-05-02 6:10 ` Tetsuo Handa 0 siblings, 1 reply; 19+ messages in thread From: Pavel Skripkin @ 2022-02-08 15:48 UTC (permalink / raw) To: Toke Høiland-Jørgensen, ath9k-devel, kvalo, davem, kuba, linville Cc: linux-wireless, netdev, linux-kernel, syzbot+03110230a11411024147, syzbot+c6dde1f690b60e0b9fbe
Hi Toke,
On 2/8/22 17:47, Toke Høiland-Jørgensen wrote: > Pavel Skripkin [email protected] writes:
Syzbot reported use-after-free Read in ath9k_hif_usb_rx_cb(). The problem was in incorrect htc_handle->drv_priv initialization.
Probable call trace which can trigger use-after-free:
ath9k_htc_probe_device() /* htc_handle->drv_priv = priv; */ ath9k_htc_wait_for_target() <— Failed ieee80211_free_hw() <— priv pointer is freed
<IRQ> … ath9k_hif_usb_rx_cb() ath9k_hif_usb_rx_stream() RX_STAT_INC() <— htc_handle->drv_priv access
In order to not add fancy protection for drv_priv we can move htc_handle->drv_priv initialization at the end of the ath9k_htc_probe_device() and add helper macro to make all *_STAT_* macros NULL save.
I’m not too familiar with how the initialisation flow of an ath9k_htc device works. Looking at htc_handle->drv_priv there seems to be three other functions apart from the stat counters that dereference it:
ath9k_htc_suspend() ath9k_htc_resume() ath9k_hif_usb_disconnect()
What guarantees that none of these will be called midway through ath9k_htc_probe_device() (which would lead to a NULL deref after this change)?
IIUC, situation you are talking about may happen even without my change. I was thinking, that ath9k_htc_probe_device() is the real ->probe() function, but things look a bit more tricky.
So, the ->probe() function may be completed before
ath9k\_htc\_probe\_device() is called, because it's called from fw loader
callback function. If ->probe() is completed, than we can call
->suspend(), ->resume() and others usb callbacks, right? And we can meet
NULL defer even if we leave drv\_priv = priv initialization on it's place.
Please, correct me if I am wrong somewhere :)
With regards,
Pavel Skripkin
^ permalink raw reply \[flat|**nested**\] 19+ messages in thread
* * \* **Re: \[PATCH v3 1/2\] ath9k: fix use-after-free in ath9k\_hif\_usb\_rx\_cb**
2022-02-08 15:48 \` Pavel Skripkin
**@ 2022-05-02 6:10 \` Tetsuo Handa**
2022-05-05 19:09 \` Pavel Skripkin
0 siblings, 1 reply; 19+ messages in thread
From: Tetsuo Handa @ 2022-05-02 6:10 UTC (permalink / raw)
To: Pavel Skripkin, Toke Høiland-Jørgensen, ath9k-devel,
kvalo, davem, kuba, linville
Cc: linux-wireless, netdev, linux-kernel,
syzbot+03110230a11411024147, syzbot+c6dde1f690b60e0b9fbe
On 2022/02/09 0:48, Pavel Skripkin wrote:
\>> ath9k\_htc\_suspend()
>> ath9k\_htc\_resume()
>> ath9k\_hif\_usb\_disconnect()
>>
>> What guarantees that none of these will be called midway through
>> ath9k\_htc\_probe\_device() (which would lead to a NULL deref after this
>> change)?
>>
>
> IIUC, situation you are talking about may happen even without my change.
> I was thinking, that ath9k\_htc\_probe\_device() is the real ->probe() function, but things look a bit more tricky.
>
>
> So, the ->probe() function may be completed before ath9k\_htc\_probe\_device()
> is called, because it's called from fw loader callback function.
Yes, ath9k\_hif\_usb\_probe() may return before complete\_all(&hif\_dev->fw\_done)
is called by ath9k\_hif\_usb\_firmware\_cb() or ath9k\_hif\_usb\_firmware\_fail().
\> If ->probe() is completed, than we can call ->suspend(), ->resume() and
> others usb callbacks, right?
Yes, but ath9k\_hif\_usb\_disconnect() and ath9k\_hif\_usb\_suspend() are calling
wait\_for\_completion(&hif\_dev->fw\_done) before checking HIF\_USB\_READY flag.
hif\_dev->fw\_done serves for serialization.
\> And we can meet NULL defer even if we leave drv\_priv = priv initialization
> on it's place.
I didn't catch the location. As long as "htc\_handle->drv\_priv = priv;" is done
before complete\_all(&hif\_dev->fw\_done) is done, is something wrong?
\>
> Please, correct me if I am wrong somewhere :)
^ permalink raw reply \[flat|**nested**\] 19+ messages in thread
* * \* **Re: \[PATCH v3 1/2\] ath9k: fix use-after-free in ath9k\_hif\_usb\_rx\_cb**
2022-05-02 6:10 \` Tetsuo Handa
**@ 2022-05-05 19:09 \` Pavel Skripkin**
2022-05-05 23:31 \` Tetsuo Handa
0 siblings, 1 reply; 19+ messages in thread
From: Pavel Skripkin @ 2022-05-05 19:09 UTC (permalink / raw)
To: Tetsuo Handa, Toke Høiland-Jørgensen, ath9k-devel,
kvalo, davem, kuba, linville
Cc: linux-wireless, netdev, linux-kernel,
syzbot+03110230a11411024147, syzbot+c6dde1f690b60e0b9fbe
\[-- Attachment #1.1: Type: text/plain, Size: 556 bytes --\]
Hi Tetsuo,
On 5/2/22 09:10, Tetsuo Handa wrote:
\>> And we can meet NULL defer even if we leave drv\_priv = priv initialization
>> on it's place.
>
> I didn't catch the location. As long as "htc\_handle->drv\_priv = priv;" is done
> before complete\_all(&hif\_dev->fw\_done) is done, is something wrong?
>
I don't really remember why I said that, but looks like I just haven't
opened callbacks' code.
My point was that my patch does not change the logic, but only fixes 2
problems: UAF and NULL deref.
With regards,
Pavel Skripkin
\[-- Attachment #2: OpenPGP digital signature --\]
\[-- Type: application/pgp-signature, Size: 840 bytes --\]
^ permalink raw reply \[flat|**nested**\] 19+ messages in thread
* * \* **Re: \[PATCH v3 1/2\] ath9k: fix use-after-free in ath9k\_hif\_usb\_rx\_cb**
2022-05-05 19:09 \` Pavel Skripkin
**@ 2022-05-05 23:31 \` Tetsuo Handa**
2022-05-10 19:26 \` Pavel Skripkin
0 siblings, 1 reply; 19+ messages in thread
From: Tetsuo Handa @ 2022-05-05 23:31 UTC (permalink / raw)
To: Pavel Skripkin, Toke Høiland-Jørgensen, ath9k-devel,
kvalo, davem, kuba, linville
Cc: linux-wireless, netdev, linux-kernel,
syzbot+03110230a11411024147, syzbot+c6dde1f690b60e0b9fbe
On 2022/05/06 4:09, Pavel Skripkin wrote:
\>>> And we can meet NULL defer even if we leave drv\_priv = priv initialization
>>> on it's place.
>>
>> I didn't catch the location. As long as "htc\_handle->drv\_priv = priv;" is done
>> before complete\_all(&hif\_dev->fw\_done) is done, is something wrong?
>>
>
> I don't really remember why I said that, but looks like I just haven't opened callbacks' code.
OK. Then, why not accept Pavel's patch?
^ permalink raw reply \[flat|**nested**\] 19+ messages in thread
* * \* **Re: \[PATCH v3 1/2\] ath9k: fix use-after-free in ath9k\_hif\_usb\_rx\_cb**
2022-05-05 23:31 \` Tetsuo Handa
**@ 2022-05-10 19:26 \` Pavel Skripkin**
2022-05-11 4:50 \` Kalle Valo
0 siblings, 1 reply; 19+ messages in thread
From: Pavel Skripkin @ 2022-05-10 19:26 UTC (permalink / raw)
To: Tetsuo Handa, Toke Høiland-Jørgensen, ath9k-devel,
kvalo, davem, kuba, linville
Cc: linux-wireless, netdev, linux-kernel,
syzbot+03110230a11411024147, syzbot+c6dde1f690b60e0b9fbe
\[-- Attachment #1.1: Type: text/plain, Size: 788 bytes --\]
Hi Tetsuo,
On 5/6/22 02:31, Tetsuo Handa wrote:
\> On 2022/05/06 4:09, Pavel Skripkin wrote:
>>>> And we can meet NULL defer even if we leave drv\_priv = priv initialization
>>>> on it's place.
>>>
>>> I didn't catch the location. As long as "htc\_handle->drv\_priv = priv;" is done
>>> before complete\_all(&hif\_dev->fw\_done) is done, is something wrong?
>>>
>>
>> I don't really remember why I said that, but looks like I just haven't opened callbacks' code.
>
> OK. Then, why not accept Pavel's patch?
As you might expect, I have same question. This series is under review
for like 7-8 months.
I have no ath9 device, so I can't test it on real hw, so somebody else
should do it for me. It's requirement to get patch accepted.
With regards,
Pavel Skripkin
\[-- Attachment #2: OpenPGP digital signature --\]
\[-- Type: application/pgp-signature, Size: 840 bytes --\]
^ permalink raw reply \[flat|**nested**\] 19+ messages in thread
* * \* **Re: \[PATCH v3 1/2\] ath9k: fix use-after-free in ath9k\_hif\_usb\_rx\_cb**
2022-05-10 19:26 \` Pavel Skripkin
**@ 2022-05-11 4:50 \` Kalle Valo**
2022-05-11 9:53 \` Toke Høiland-Jørgensen
0 siblings, 1 reply; 19+ messages in thread
From: Kalle Valo @ 2022-05-11 4:50 UTC (permalink / raw)
To: Pavel Skripkin
Cc: Tetsuo Handa, Toke Høiland-Jørgensen, ath9k-devel,
davem, kuba, linville, linux-wireless, netdev, linux-kernel,
syzbot+03110230a11411024147, syzbot+c6dde1f690b60e0b9fbe
Pavel Skripkin <[email protected]> writes:
\> Hi Tetsuo,
>
> On 5/6/22 02:31, Tetsuo Handa wrote:
>> On 2022/05/06 4:09, Pavel Skripkin wrote:
>>>>> And we can meet NULL defer even if we leave drv\_priv = priv initialization
>>>>> on it's place.
>>>>
>>>> I didn't catch the location. As long as "htc\_handle->drv\_priv = priv;" is done
>>>> before complete\_all(&hif\_dev->fw\_done) is done, is something wrong?
>>>>
>>>
>>> I don't really remember why I said that, but looks like I just haven't opened callbacks' code.
>>
>> OK. Then, why not accept Pavel's patch?
>
> As you might expect, I have same question. This series is under review
> for like 7-8 months.
>
> I have no ath9 device, so I can't test it on real hw, so somebody else
> should do it for me. It's requirement to get patch accepted.
As Toke stepped up to be the ath9k maintainer the situation with ath9k
is now much better. I recommend resubmitting any ath9k patches you might
have.
--
https://patchwork.kernel.org/project/linux-wireless/list/
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
^ permalink raw reply \[flat|**nested**\] 19+ messages in thread
* * \* **Re: \[PATCH v3 1/2\] ath9k: fix use-after-free in ath9k\_hif\_usb\_rx\_cb**
2022-05-11 4:50 \` Kalle Valo
**@ 2022-05-11 9:53 \` Toke Høiland-Jørgensen**
2022-05-11 9:59 \` Kalle Valo
0 siblings, 1 reply; 19+ messages in thread
From: Toke Høiland-Jørgensen @ 2022-05-11 9:53 UTC (permalink / raw)
To: Kalle Valo, Pavel Skripkin
Cc: Tetsuo Handa, ath9k-devel, davem, kuba, linville, linux-wireless,
netdev, linux-kernel, syzbot+03110230a11411024147,
syzbot+c6dde1f690b60e0b9fbe
Kalle Valo <[email protected]> writes:
\> Pavel Skripkin <[email protected]> writes:
>
>> Hi Tetsuo,
>>
>> On 5/6/22 02:31, Tetsuo Handa wrote:
>>> On 2022/05/06 4:09, Pavel Skripkin wrote:
>>>>>> And we can meet NULL defer even if we leave drv\_priv = priv initialization
>>>>>> on it's place.
>>>>>
>>>>> I didn't catch the location. As long as "htc\_handle->drv\_priv = priv;" is done
>>>>> before complete\_all(&hif\_dev->fw\_done) is done, is something wrong?
>>>>>
>>>>
>>>> I don't really remember why I said that, but looks like I just haven't opened callbacks' code.
>>>
>>> OK. Then, why not accept Pavel's patch?
>>
>> As you might expect, I have same question. This series is under review
>> for like 7-8 months.
>>
>> I have no ath9 device, so I can't test it on real hw, so somebody else
>> should do it for me. It's requirement to get patch accepted.
>
> As Toke stepped up to be the ath9k maintainer the situation with ath9k
> is now much better. I recommend resubmitting any ath9k patches you might
> have.
No need to resubmit this one, it's still in patchwork waiting for me to
take a closer look. I have a conference this week, but should hopefully
have some time for this next week.
-Toke
^ permalink raw reply \[flat|**nested**\] 19+ messages in thread
* * \* **Re: \[PATCH v3 1/2\] ath9k: fix use-after-free in ath9k\_hif\_usb\_rx\_cb**
2022-05-11 9:53 \` Toke Høiland-Jørgensen
**@ 2022-05-11 9:59 \` Kalle Valo**
0 siblings, 0 replies; 19+ messages in thread
From: Kalle Valo @ 2022-05-11 9:59 UTC (permalink / raw)
To: Toke Høiland-Jørgensen
Cc: Pavel Skripkin, Tetsuo Handa, ath9k-devel, davem, kuba, linville,
linux-wireless, netdev, linux-kernel,
syzbot+03110230a11411024147, syzbot+c6dde1f690b60e0b9fbe
Toke Høiland-Jørgensen <[email protected]> writes:
\> Kalle Valo <[email protected]> writes:
>
>> Pavel Skripkin <[email protected]> writes:
>>
>>> Hi Tetsuo,
>>>
>>> On 5/6/22 02:31, Tetsuo Handa wrote:
>>>> On 2022/05/06 4:09, Pavel Skripkin wrote:
>>>>>>> And we can meet NULL defer even if we leave drv\_priv = priv initialization
>>>>>>> on it's place.
>>>>>>
>>>>>> I didn't catch the location. As long as "htc\_handle->drv\_priv = priv;" is done
>>>>>> before complete\_all(&hif\_dev->fw\_done) is done, is something wrong?
>>>>>>
>>>>>
>>>>> I don't really remember why I said that, but looks like I just
>>>>> haven't opened callbacks' code.
>>>>
>>>> OK. Then, why not accept Pavel's patch?
>>>
>>> As you might expect, I have same question. This series is under review
>>> for like 7-8 months.
>>>
>>> I have no ath9 device, so I can't test it on real hw, so somebody else
>>> should do it for me. It's requirement to get patch accepted.
>>
>> As Toke stepped up to be the ath9k maintainer the situation with ath9k
>> is now much better. I recommend resubmitting any ath9k patches you might
>> have.
>
> No need to resubmit this one, it's still in patchwork waiting for me to
> take a closer look.
Ah sorry, I thought this was something which was submitted 7-8 months
ago but I didn't check, I should have.
\> I have a conference this week, but should hopefully have some time for
> this next week.
It's great to be able to start meeting people again, have a good one :)
--
https://patchwork.kernel.org/project/linux-wireless/list/
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
^ permalink raw reply \[flat|**nested**\] 19+ messages in thread
* Re: [PATCH v3 1/2] ath9k: fix use-after-free in ath9k_hif_usb_rx_cb 2022-02-07 20:24 [PATCH v3 1/2] ath9k: fix use-after-free in ath9k_hif_usb_rx_cb Pavel Skripkin 2022-02-07 20:24 ` [PATCH v3 2/2] ath9k: htc: clean up *STAT_* macros Pavel Skripkin 2022-02-08 14:47 ` [PATCH v3 1/2] ath9k: fix use-after-free in ath9k_hif_usb_rx_cb Toke Høiland-Jørgensen @ 2022-05-12 12:49 ` Toke Høiland-Jørgensen 2022-05-12 12:55 ` Pavel Skripkin 2022-05-12 16:05 ` Jeff Johnson 3 siblings, 1 reply; 19+ messages in thread From: Toke Høiland-Jørgensen @ 2022-05-12 12:49 UTC (permalink / raw) To: Pavel Skripkin, ath9k-devel, kvalo, davem, kuba, linville Cc: linux-wireless, netdev, linux-kernel, Pavel Skripkin, syzbot+03110230a11411024147, syzbot+c6dde1f690b60e0b9fbe
Pavel Skripkin [email protected] writes:
> Syzbot reported use-after-free Read in ath9k_hif_usb_rx_cb(). The
problem was in incorrect htc_handle->drv_priv initialization.
Probable call trace which can trigger use-after-free:
ath9k_htc_probe_device() /* htc_handle->drv_priv = priv; */ ath9k_htc_wait_for_target() <— Failed ieee80211_free_hw() <— priv pointer is freed
<IRQ> … ath9k_hif_usb_rx_cb() ath9k_hif_usb_rx_stream() RX_STAT_INC() <— htc_handle->drv_priv access
In order to not add fancy protection for drv_priv we can move htc_handle->drv_priv initialization at the end of the ath9k_htc_probe_device() and add helper macro to make all *_STAT_* macros NULL save.
Fixes: fb9987d0f748 (“ath9k_htc: Support for AR9271 chipset.”) Reported-and-tested-by: [email protected] Reported-and-tested-by: [email protected] Signed-off-by: Pavel Skripkin [email protected] Could you link the original syzbot report in the commit message as well, please? Also that ‘tested-by’ implies that syzbot run-tested this? How does it do that; does it have ath9k_htc hardware?
> —
Changes since v2:
- My send-email script forgot, that mailing lists exist. Added back all related lists
Changes since v1:
- Removed clean-ups and moved them to 2/2
drivers/net/wireless/ath/ath9k/htc.h | 10 ++++±---- drivers/net/wireless/ath/ath9k/htc_drv_init.c | 3 +± 2 files changed, 7 insertions(+), 6 deletions(-)
diff --git a/drivers/net/wireless/ath/ath9k/htc.h b/drivers/net/wireless/ath/ath9k/htc.h index 6b45e63fae4b…141642e5e00d 100644 — a/drivers/net/wireless/ath/ath9k/htc.h +++ b/drivers/net/wireless/ath/ath9k/htc.h @@ -327,11 +327,11 @@ static inline struct ath9k_htc_tx_ctl *HTC_SKB_CB(struct sk_buff *skb) }
#ifdef CONFIG_ATH9K_HTC_DEBUGFS
-#define TX_STAT_INC© (hif_dev->htc_handle->drv_priv->debug.tx_stats.c++) -#define TX_STAT_ADD(c, a) (hif_dev->htc_handle->drv_priv->debug.tx_stats.c += a) -#define RX_STAT_INC© (hif_dev->htc_handle->drv_priv->debug.skbrx_stats.c++) -#define RX_STAT_ADD(c, a) (hif_dev->htc_handle->drv_priv->debug.skbrx_stats.c += a) +#define __STAT_SAVE(expr) (hif_dev->htc_handle->drv_priv ? (expr) : 0) +#define TX_STAT_INC© __STAT_SAVE(hif_dev->htc_handle->drv_priv->debug.tx_stats.c++) +#define TX_STAT_ADD(c, a) __STAT_SAVE(hif_dev->htc_handle->drv_priv->debug.tx_stats.c += a) +#define RX_STAT_INC© __STAT_SAVE(hif_dev->htc_handle->drv_priv->debug.skbrx_stats.c++) +#define RX_STAT_ADD(c, a) __STAT_SAVE(hif_dev->htc_handle->drv_priv->debug.skbrx_stats.c += a) #define CAB_STAT_INC priv->debug.tx_stats.cab_queued++ s/SAVE/SAFE/ here and in the next patch (and the commit message).
-Toke
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v3 1/2] ath9k: fix use-after-free in ath9k_hif_usb_rx_cb 2022-05-12 12:49 ` Toke Høiland-Jørgensen @ 2022-05-12 12:55 ` Pavel Skripkin 2022-05-12 13:48 ` Toke Høiland-Jørgensen 0 siblings, 1 reply; 19+ messages in thread From: Pavel Skripkin @ 2022-05-12 12:55 UTC (permalink / raw) To: Toke Høiland-Jørgensen, ath9k-devel, kvalo, davem, kuba, linville Cc: linux-wireless, netdev, linux-kernel, syzbot+03110230a11411024147, syzbot+c6dde1f690b60e0b9fbe
[-- Attachment #1.1: Type: text/plain, Size: 2925 bytes --]
Hi Toke,
On 5/12/22 15:49, Toke Høiland-Jørgensen wrote: > Pavel Skripkin [email protected] writes:
Syzbot reported use-after-free Read in ath9k_hif_usb_rx_cb(). The problem was in incorrect htc_handle->drv_priv initialization.
Probable call trace which can trigger use-after-free:
ath9k_htc_probe_device() /* htc_handle->drv_priv = priv; */ ath9k_htc_wait_for_target() <— Failed ieee80211_free_hw() <— priv pointer is freed
<IRQ> … ath9k_hif_usb_rx_cb() ath9k_hif_usb_rx_stream() RX_STAT_INC() <— htc_handle->drv_priv access
In order to not add fancy protection for drv_priv we can move htc_handle->drv_priv initialization at the end of the ath9k_htc_probe_device() and add helper macro to make all *_STAT_* macros NULL save.
Fixes: fb9987d0f748 (“ath9k_htc: Support for AR9271 chipset.”) Reported-and-tested-by: [email protected] Reported-and-tested-by: [email protected] Signed-off-by: Pavel Skripkin [email protected]
Could you link the original syzbot report in the commit message as well, Sure! See links below
use-after-free bug: https://syzkaller.appspot.com/bug?id=6ead44e37afb6866ac0c7dd121b4ce07cb665f60
NULL deref bug: https://syzkaller.appspot.com/bug?id=b8101ffcec107c0567a0cd8acbbacec91e9ee8de
I can add them in commit message if you want :)
> please? Also that ‘tested-by’ implies that syzbot run-tested this? How
does it do that; does it have ath9k_htc hardware?
No, it uses CONFIG_USB_RAW_GADGET and CONFIG_USB_DUMMY_HCD for gadgets for emulating usb devices.
Basically these things “connect” fake USB device with random usb ids from hardcoded table and try to do various things with usb driver
>> — [snip]
>> -#define TX_STAT_INC© (hif_dev->htc_handle->drv_priv->debug.tx_stats.c++)
-#define TX_STAT_ADD(c, a) (hif_dev->htc_handle->drv_priv->debug.tx_stats.c += a) -#define RX_STAT_INC© (hif_dev->htc_handle->drv_priv->debug.skbrx_stats.c++) -#define RX_STAT_ADD(c, a) (hif_dev->htc_handle->drv_priv->debug.skbrx_stats.c += a) +#define __STAT_SAVE(expr) (hif_dev->htc_handle->drv_priv ? (expr) : 0) +#define TX_STAT_INC© __STAT_SAVE(hif_dev->htc_handle->drv_priv->debug.tx_stats.c++) +#define TX_STAT_ADD(c, a) __STAT_SAVE(hif_dev->htc_handle->drv_priv->debug.tx_stats.c += a) +#define RX_STAT_INC© __STAT_SAVE(hif_dev->htc_handle->drv_priv->debug.skbrx_stats.c++) +#define RX_STAT_ADD(c, a) __STAT_SAVE(hif_dev->htc_handle->drv_priv->debug.skbrx_stats.c += a) #define CAB_STAT_INC priv->debug.tx_stats.cab_queued++
s/SAVE/SAFE/ here and in the next patch (and the commit message).
Oh, sorry about that! Will update in next version
With regards,
Pavel Skripkin
\[-- Attachment #2: OpenPGP digital signature --\]
\[-- Type: application/pgp-signature, Size: 840 bytes --\]
^ permalink raw reply \[flat|**nested**\] 19+ messages in thread
* * \* **Re: \[PATCH v3 1/2\] ath9k: fix use-after-free in ath9k\_hif\_usb\_rx\_cb**
2022-05-12 12:55 \` Pavel Skripkin
**@ 2022-05-12 13:48 \` Toke Høiland-Jørgensen**
0 siblings, 0 replies; 19+ messages in thread
From: Toke Høiland-Jørgensen @ 2022-05-12 13:48 UTC (permalink / raw)
To: Pavel Skripkin, ath9k-devel, kvalo, davem, kuba, linville
Cc: linux-wireless, netdev, linux-kernel,
syzbot+03110230a11411024147, syzbot+c6dde1f690b60e0b9fbe
Pavel Skripkin <[email protected]> writes:
\> Hi Toke,
>
> On 5/12/22 15:49, Toke Høiland-Jørgensen wrote:
>> Pavel Skripkin <[email protected]> writes:
>>
>>> Syzbot reported use-after-free Read in ath9k\_hif\_usb\_rx\_cb(). The
>>> problem was in incorrect htc\_handle->drv\_priv initialization.
>>>
>>> Probable call trace which can trigger use-after-free:
>>>
>>> ath9k\_htc\_probe\_device()
>>> /\* htc\_handle->drv\_priv = priv; \*/
>>> ath9k\_htc\_wait\_for\_target() <--- Failed
>>> ieee80211\_free\_hw() <--- priv pointer is freed
>>>
>>> <IRQ>
>>> ...
>>> ath9k\_hif\_usb\_rx\_cb()
>>> ath9k\_hif\_usb\_rx\_stream()
>>> RX\_STAT\_INC() <--- htc\_handle->drv\_priv access
>>>
>>> In order to not add fancy protection for drv\_priv we can move
>>> htc\_handle->drv\_priv initialization at the end of the
>>> ath9k\_htc\_probe\_device() and add helper macro to make
>>> all \*\_STAT\_\* macros NULL save.
>>>
>>> Fixes: fb9987d0f748 ("ath9k\_htc: Support for AR9271 chipset.")
>>> Reported-and-tested-by: [email protected]
>>> Reported-and-tested-by: [email protected]
>>> Signed-off-by: Pavel Skripkin <[email protected]>
>>
>> Could you link the original syzbot report in the commit message as well,
>
> Sure! See links below
>
> use-after-free bug:
> https://syzkaller.appspot.com/bug?id=6ead44e37afb6866ac0c7dd121b4ce07cb665f60
>
> NULL deref bug:
> https://syzkaller.appspot.com/bug?id=b8101ffcec107c0567a0cd8acbbacec91e9ee8de
>
> I can add them in commit message if you want :)
Yes, please do!
\>> please? Also that 'tested-by' implies that syzbot run-tested this? How
>> does it do that; does it have ath9k\_htc hardware?
>>
>
> No, it uses CONFIG\_USB\_RAW\_GADGET and CONFIG\_USB\_DUMMY\_HCD for gadgets
> for emulating usb devices.
>
> Basically these things "connect" fake USB device with random usb ids
> from hardcoded table and try to do various things with usb driver
Ah, right, hence the failures I suppose? Makes sense.
\> \[snip\]
>
>>> -#define TX\_STAT\_INC(c) (hif\_dev->htc\_handle->drv\_priv->debug.tx\_stats.c++)
>>> -#define TX\_STAT\_ADD(c, a) (hif\_dev->htc\_handle->drv\_priv->debug.tx\_stats.c += a)
>>> -#define RX\_STAT\_INC(c) (hif\_dev->htc\_handle->drv\_priv->debug.skbrx\_stats.c++)
>>> -#define RX\_STAT\_ADD(c, a) (hif\_dev->htc\_handle->drv\_priv->debug.skbrx\_stats.c += a)
>>> +#define \_\_STAT\_SAVE(expr) (hif\_dev->htc\_handle->drv\_priv ? (expr) : 0)
>>> +#define TX\_STAT\_INC(c) \_\_STAT\_SAVE(hif\_dev->htc\_handle->drv\_priv->debug.tx\_stats.c++)
>>> +#define TX\_STAT\_ADD(c, a) \_\_STAT\_SAVE(hif\_dev->htc\_handle->drv\_priv->debug.tx\_stats.c += a)
>>> +#define RX\_STAT\_INC(c) \_\_STAT\_SAVE(hif\_dev->htc\_handle->drv\_priv->debug.skbrx\_stats.c++)
>>> +#define RX\_STAT\_ADD(c, a) \_\_STAT\_SAVE(hif\_dev->htc\_handle->drv\_priv->debug.skbrx\_stats.c += a)
>>> #define CAB\_STAT\_INC priv->debug.tx\_stats.cab\_queued++
>>
>> s/SAVE/SAFE/ here and in the next patch (and the commit message).
>>
>
> Oh, sorry about that! Will update in next version
Thanks! Other than that, I think the patch looks reasonable...
-Toke
^ permalink raw reply \[flat|**nested**\] 19+ messages in thread
* Re: [PATCH v3 1/2] ath9k: fix use-after-free in ath9k_hif_usb_rx_cb 2022-02-07 20:24 [PATCH v3 1/2] ath9k: fix use-after-free in ath9k_hif_usb_rx_cb Pavel Skripkin ` (2 preceding siblings …) 2022-05-12 12:49 ` Toke Høiland-Jørgensen @ 2022-05-12 16:05 ` Jeff Johnson 2022-05-12 16:09 ` Pavel Skripkin 3 siblings, 1 reply; 19+ messages in thread From: Jeff Johnson @ 2022-05-12 16:05 UTC (permalink / raw) To: Pavel Skripkin, ath9k-devel, kvalo, davem, kuba, toke, linville Cc: linux-wireless, netdev, linux-kernel, syzbot+03110230a11411024147, syzbot+c6dde1f690b60e0b9fbe
On 2/7/2022 12:24 PM, Pavel Skripkin wrote: […snip…] >
#ifdef CONFIG_ATH9K_HTC_DEBUGFS
-#define TX_STAT_INC© (hif_dev->htc_handle->drv_priv->debug.tx_stats.c++) -#define TX_STAT_ADD(c, a) (hif_dev->htc_handle->drv_priv->debug.tx_stats.c += a) -#define RX_STAT_INC© (hif_dev->htc_handle->drv_priv->debug.skbrx_stats.c++) -#define RX_STAT_ADD(c, a) (hif_dev->htc_handle->drv_priv->debug.skbrx_stats.c += a) +#define __STAT_SAVE(expr) (hif_dev->htc_handle->drv_priv ? (expr) : 0) +#define TX_STAT_INC© __STAT_SAVE(hif_dev->htc_handle->drv_priv->debug.tx_stats.c++) +#define TX_STAT_ADD(c, a) __STAT_SAVE(hif_dev->htc_handle->drv_priv->debug.tx_stats.c += a) +#define RX_STAT_INC© __STAT_SAVE(hif_dev->htc_handle->drv_priv->debug.skbrx_stats.c++) +#define RX_STAT_ADD(c, a) __STAT_SAVE(hif_dev->htc_handle->drv_priv->debug.skbrx_stats.c += a) it is unfortunate that the existing macros don’t abide by the coding style: Things to avoid when using macros: macros that depend on having a local variable with a magic name
the companion macros in ath9k/debug.h do the right thing
perhaps this could be given to Kernel Janitors for subsequent cleanup?
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v3 1/2] ath9k: fix use-after-free in ath9k_hif_usb_rx_cb 2022-05-12 16:05 ` Jeff Johnson @ 2022-05-12 16:09 ` Pavel Skripkin 0 siblings, 0 replies; 19+ messages in thread From: Pavel Skripkin @ 2022-05-12 16:09 UTC (permalink / raw) To: Jeff Johnson, ath9k-devel, kvalo, davem, kuba, toke, linville Cc: linux-wireless, netdev, linux-kernel, syzbot+03110230a11411024147, syzbot+c6dde1f690b60e0b9fbe
[-- Attachment #1.1: Type: text/plain, Size: 1485 bytes --]
Hi Jeff,
On 5/12/22 19:05, Jeff Johnson wrote: > On 2/7/2022 12:24 PM, Pavel Skripkin wrote:
[…snip…]
#ifdef CONFIG_ATH9K_HTC_DEBUGFS
-#define TX_STAT_INC© (hif_dev->htc_handle->drv_priv->debug.tx_stats.c++) -#define TX_STAT_ADD(c, a) (hif_dev->htc_handle->drv_priv->debug.tx_stats.c += a) -#define RX_STAT_INC© (hif_dev->htc_handle->drv_priv->debug.skbrx_stats.c++) -#define RX_STAT_ADD(c, a) (hif_dev->htc_handle->drv_priv->debug.skbrx_stats.c += a) +#define __STAT_SAVE(expr) (hif_dev->htc_handle->drv_priv ? (expr) : 0) +#define TX_STAT_INC© __STAT_SAVE(hif_dev->htc_handle->drv_priv->debug.tx_stats.c++) +#define TX_STAT_ADD(c, a) __STAT_SAVE(hif_dev->htc_handle->drv_priv->debug.tx_stats.c += a) +#define RX_STAT_INC© __STAT_SAVE(hif_dev->htc_handle->drv_priv->debug.skbrx_stats.c++) +#define RX_STAT_ADD(c, a) __STAT_SAVE(hif_dev->htc_handle->drv_priv->debug.skbrx_stats.c += a)
it is unfortunate that the existing macros don’t abide by the coding style: Things to avoid when using macros: macros that depend on having a local variable with a magic name
the companion macros in ath9k/debug.h do the right thing
perhaps this could be given to Kernel Janitors for subsequent cleanup? Thanks for pointing that out!
I will clean them up in next version. I think, it will be much easier than proposing this task to Kernel Janitors.
With regards,
Pavel Skripkin
\[-- Attachment #2: OpenPGP digital signature --\]
\[-- Type: application/pgp-signature, Size: 840 bytes --\]
^ permalink raw reply \[flat|**nested**\] 19+ messages in thread
end of thread, other threads:[~2022-05-12 16:09 UTC | newest]
Thread overview: 19+ messages (download: mbox.gz / follow: Atom feed) – links below jump to the message on this page – 2022-02-07 20:24 [PATCH v3 1/2] ath9k: fix use-after-free in ath9k_hif_usb_rx_cb Pavel Skripkin 2022-02-07 20:24 ` [PATCH v3 2/2] ath9k: htc: clean up *STAT_* macros Pavel Skripkin 2022-02-07 21:24 ` Jeff Johnson 2022-02-08 15:32 ` Toke Høiland-Jørgensen 2022-02-08 15:49 ` Pavel Skripkin 2022-02-08 14:47 ` [PATCH v3 1/2] ath9k: fix use-after-free in ath9k_hif_usb_rx_cb Toke Høiland-Jørgensen 2022-02-08 15:48 ` Pavel Skripkin 2022-05-02 6:10 ` Tetsuo Handa 2022-05-05 19:09 ` Pavel Skripkin 2022-05-05 23:31 ` Tetsuo Handa 2022-05-10 19:26 ` Pavel Skripkin 2022-05-11 4:50 ` Kalle Valo 2022-05-11 9:53 ` Toke Høiland-Jørgensen 2022-05-11 9:59 ` Kalle Valo 2022-05-12 12:49 ` Toke Høiland-Jørgensen 2022-05-12 12:55 ` Pavel Skripkin 2022-05-12 13:48 ` Toke Høiland-Jørgensen 2022-05-12 16:05 ` Jeff Johnson 2022-05-12 16:09 ` Pavel Skripkin
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).
Related news
Red Hat Security Advisory 2023-3495-01 - Logging Subsystem 5.7.2 - Red Hat OpenShift. Issues addressed include cross site scripting and denial of service vulnerabilities.
Red Hat Security Advisory 2023-3356-01 - Red Hat Advanced Cluster Management for Kubernetes 2.5.9 images Red Hat Advanced Cluster Management for Kubernetes provides the capabilities to address common challenges that administrators and site reliability engineers face as they work across a range of public and private cloud environments. Clusters and applications are all visible and managed from a single console—with security policy built in. This advisory contains the container images for Red Hat Advanced Cluster Management for Kubernetes, which fix several bugs.
Red Hat Security Advisory 2023-3326-01 - Red Hat Advanced Cluster Management for Kubernetes 2.6.6 images. This advisory contains the container images for Red Hat Advanced Cluster Management for Kubernetes, which fix several bugs.
An update for kernel is now available for Red Hat Enterprise Linux 8. Red Hat Product Security has rated this update as having a security impact of Important. A Common Vulnerability Scoring System (CVSS) base score, which gives a detailed severity rating, is available for each vulnerability from the CVE link(s) in the References section.This content is licensed under the Creative Commons Attribution 4.0 International License (https://creativecommons.org/licenses/by/4.0/). If you distribute this content, or a modified version of it, you must provide attribution to Red Hat Inc. and provide a link to the original. Related CVEs: * CVE-2021-26341: A flaw was found in hw. This issue can cause AMD CPUs to transiently execute beyond unconditional direct branches. * CVE-2021-33655: An out-of-bounds write flaw was found in the Linux kernel’s framebuffer-based console driver functionality in the way a user triggers ioctl FBIOPUT_VSCREENINFO with malicious data. This flaw allows a local user to c...
An update for kernel-rt is now available for Red Hat Enterprise Linux 8. Red Hat Product Security has rated this update as having a security impact of Important. A Common Vulnerability Scoring System (CVSS) base score, which gives a detailed severity rating, is available for each vulnerability from the CVE link(s) in the References section.This content is licensed under the Creative Commons Attribution 4.0 International License (https://creativecommons.org/licenses/by/4.0/). If you distribute this content, or a modified version of it, you must provide attribution to Red Hat Inc. and provide a link to the original. Related CVEs: * CVE-2021-26341: A flaw was found in hw. This issue can cause AMD CPUs to transiently execute beyond unconditional direct branches. * CVE-2021-33655: An out-of-bounds write flaw was found in the Linux kernel’s framebuffer-based console driver functionality in the way a user triggers ioctl FBIOPUT_VSCREENINFO with malicious data. This flaw allows a local user t...
Dell VxRail, versions prior to 7.0.410, contain a Container Escape Vulnerability. A local high-privileged attacker could potentially exploit this vulnerability, leading to the execution of arbitrary OS commands on the container's underlying OS. Exploitation may lead to a system take over by an attacker.
Hello everyone! Great news for my open source Scanvus project! You can now perform vulnerability checks on Linux hosts and docker images not only using the Vulners.com API, but also with the Vulns.io VM API. It’s especially nice that all the code to support the new API was written and contributed by colleagues from Vulns.io. […]
Red Hat Security Advisory 2022-7933-01 - The kernel-rt packages provide the Real Time Linux Kernel, which enables fine-tuning for systems with extremely high determinism requirements. Issues addressed include code execution, denial of service, double free, information leakage, null pointer, out of bounds access, out of bounds write, privilege escalation, and use-after-free vulnerabilities.
An update for kernel is now available for Red Hat Enterprise Linux 9. Red Hat Product Security has rated this update as having a security impact of Moderate. A Common Vulnerability Scoring System (CVSS) base score, which gives a detailed severity rating, is available for each vulnerability from the CVE link(s) in the References section.This content is licensed under the Creative Commons Attribution 4.0 International License (https://creativecommons.org/licenses/by/4.0/). If you distribute this content, or a modified version of it, you must provide attribution to Red Hat Inc. and provide a link to the original. Related CVEs: * CVE-2020-36516: kernel: off-path attacker may inject data or terminate victim's TCP session * CVE-2021-3640: kernel: use-after-free vulnerability in function sco_sock_sendmsg() * CVE-2022-0168: kernel: smb2_ioctl_query_info NULL pointer dereference * CVE-2022-0617: kernel: NULL pointer dereference in udf_expand_file_adinicbdue() during writeback * CVE-2022-0854: ...
An update for kernel-rt is now available for Red Hat Enterprise Linux 9. Red Hat Product Security has rated this update as having a security impact of Moderate. A Common Vulnerability Scoring System (CVSS) base score, which gives a detailed severity rating, is available for each vulnerability from the CVE link(s) in the References section.This content is licensed under the Creative Commons Attribution 4.0 International License (https://creativecommons.org/licenses/by/4.0/). If you distribute this content, or a modified version of it, you must provide attribution to Red Hat Inc. and provide a link to the original. Related CVEs: * CVE-2020-36516: kernel: off-path attacker may inject data or terminate victim's TCP session * CVE-2021-3640: kernel: use-after-free vulnerability in function sco_sock_sendmsg() * CVE-2022-0168: kernel: smb2_ioctl_query_info NULL pointer dereference * CVE-2022-0617: kernel: NULL pointer dereference in udf_expand_file_adinicbdue() during writeback * CVE-2022-085...
Ubuntu Security Notice 5582-1 - Arthur Mongodin discovered that the netfilter subsystem in the Linux kernel did not properly perform data validation. A local attacker could use this to escalate privileges in certain situations. Zhenpeng Lin discovered that the network packet scheduler implementation in the Linux kernel did not properly remove all references to a route filter before freeing it in some situations. A local attacker could use this to cause a denial of service or execute arbitrary code.
Ubuntu Security Notice 5566-1 - Zhenpeng Lin discovered that the network packet scheduler implementation in the Linux kernel did not properly remove all references to a route filter before freeing it in some situations. A local attacker could use this to cause a denial of service or execute arbitrary code. It was discovered that the netfilter subsystem of the Linux kernel did not prevent one nft object from referencing an nft set in another nft table, leading to a use-after-free vulnerability. A local attacker could use this to cause a denial of service or execute arbitrary code.
Ubuntu Security Notice 5564-1 - Zhenpeng Lin discovered that the network packet scheduler implementation in the Linux kernel did not properly remove all references to a route filter before freeing it in some situations. A local attacker could use this to cause a denial of service or execute arbitrary code. It was discovered that the netfilter subsystem of the Linux kernel did not prevent one nft object from referencing an nft set in another nft table, leading to a use-after-free vulnerability. A local attacker could use this to cause a denial of service or execute arbitrary code.
Ubuntu Security Notice 5562-1 - Zhenpeng Lin discovered that the network packet scheduler implementation in the Linux kernel did not properly remove all references to a route filter before freeing it in some situations. A local attacker could use this to cause a denial of service or execute arbitrary code. It was discovered that the netfilter subsystem of the Linux kernel did not prevent one nft object from referencing an nft set in another nft table, leading to a use-after-free vulnerability. A local attacker could use this to cause a denial of service or execute arbitrary code.
Ubuntu Security Notice 5560-2 - Zhenpeng Lin discovered that the network packet scheduler implementation in the Linux kernel did not properly remove all references to a route filter before freeing it in some situations. A local attacker could use this to cause a denial of service or execute arbitrary code. It was discovered that the netfilter subsystem of the Linux kernel did not prevent one nft object from referencing an nft set in another nft table, leading to a use-after-free vulnerability. A local attacker could use this to cause a denial of service or execute arbitrary code.
Ubuntu Security Notice 5560-1 - Zhenpeng Lin discovered that the network packet scheduler implementation in the Linux kernel did not properly remove all references to a route filter before freeing it in some situations. A local attacker could use this to cause a denial of service or execute arbitrary code. It was discovered that the netfilter subsystem of the Linux kernel did not prevent one nft object from referencing an nft set in another nft table, leading to a use-after-free vulnerability. A local attacker could use this to cause a denial of service or execute arbitrary code.
Ubuntu Security Notice 5544-1 - It was discovered that the Atheros ath9k wireless device driver in the Linux kernel did not properly handle some error conditions, leading to a use-after-free vulnerability. A local attacker could use this to cause a denial of service or possibly execute arbitrary code. Felix Fu discovered that the Sun RPC implementation in the Linux kernel did not properly handle socket states, leading to a use-after-free vulnerability. A remote attacker could possibly use this to cause a denial of service or execute arbitrary code.
Ubuntu Security Notice 5529-1 - It was discovered that the Atheros ath9k wireless device driver in the Linux kernel did not properly handle some error conditions, leading to a use-after-free vulnerability. A local attacker could use this to cause a denial of service or possibly execute arbitrary code. Yongkang Jia discovered that the KVM hypervisor implementation in the Linux kernel did not properly handle guest TLB mapping invalidation requests in some situations. An attacker in a guest VM could use this to cause a denial of service in the host OS.
Ubuntu Security Notice 5517-1 - It was discovered that the Atheros ath9k wireless device driver in the Linux kernel did not properly handle some error conditions, leading to a use-after-free vulnerability. A local attacker could use this to cause a denial of service or possibly execute arbitrary code. It was discovered that the virtio RPMSG bus driver in the Linux kernel contained a double-free vulnerability in certain error conditions. A local attacker could possibly use this to cause a denial of service.
Ubuntu Security Notice 5513-1 - Norbert Slusarek discovered a race condition in the CAN BCM networking protocol of the Linux kernel leading to multiple use-after-free vulnerabilities. A local attacker could use this issue to execute arbitrary code. Likang Luo discovered that a race condition existed in the Bluetooth subsystem of the Linux kernel, leading to a use-after-free vulnerability. A local attacker could use this to cause a denial of service or possibly execute arbitrary code.
Ubuntu Security Notice 5505-1 - Norbert Slusarek discovered a race condition in the CAN BCM networking protocol of the Linux kernel leading to multiple use-after-free vulnerabilities. A local attacker could use this issue to execute arbitrary code. Likang Luo discovered that a race condition existed in the Bluetooth subsystem of the Linux kernel, leading to a use-after-free vulnerability. A local attacker could use this to cause a denial of service or possibly execute arbitrary code.
Ubuntu Security Notice 5500-1 - Eric Biederman discovered that the cgroup process migration implementation in the Linux kernel did not perform permission checks correctly in some situations. A local attacker could possibly use this to gain administrative privileges. Lin Ma discovered that the NFC Controller Interface implementation in the Linux kernel contained a race condition, leading to a use-after-free vulnerability. A local attacker could use this to cause a denial of service or possibly execute arbitrary code.