Syzbot reported use-after-free Read in ath9k_hif_usb_rx_cb() [0]. 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 safe, since syzbot has reported related NULL
deref in that macros [1]
Link: https://syzkaller.appspot.com/bug?id=6ead44e37afb6866ac0c7dd121b4ce07cb665f60 [0]
Link: https://syzkaller.appspot.com/bug?id=b8101ffcec107c0567a0cd8acbbacec91e9ee8de [1]
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 v4:
s/save/safe/ in commit message
Changes since v3:
- s/SAVE/SAFE/
- Added links to syzkaller reports
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..e3d546ef71dd 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(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_SAFE(expr) (hif_dev->htc_handle->drv_priv ? (expr) : 0)
+#define TX_STAT_INC(c) __STAT_SAFE(hif_dev->htc_handle->drv_priv->debug.tx_stats.c++)
+#define TX_STAT_ADD(c, a) __STAT_SAFE(hif_dev->htc_handle->drv_priv->debug.tx_stats.c += a)
+#define RX_STAT_INC(c) __STAT_SAFE(hif_dev->htc_handle->drv_priv->debug.skbrx_stats.c++)
+#define RX_STAT_ADD(c, a) __STAT_SAFE(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.36.1
On Sat, 21 May 2022 23:28:01 +0200,
Pavel Skripkin wrote:
>
> Syzbot reported use-after-free Read in ath9k_hif_usb_rx_cb() [0]. 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 safe, since syzbot has reported related NULL
> deref in that macros [1]
>
> Link: https://syzkaller.appspot.com/bug?id=6ead44e37afb6866ac0c7dd121b4ce07cb665f60 [0]
> Link: https://syzkaller.appspot.com/bug?id=b8101ffcec107c0567a0cd8acbbacec91e9ee8de [1]
> 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]>
Hi Toke, any update on this?
I'm asking it because this is a fix for a security issue
(CVE-2022-1679 [*]), and distros have been waiting for the fix getting
merged to the upstream for weeks.
[*] https://nvd.nist.gov/vuln/detail/CVE-2022-1679
thanks,
Takashi
[ This had the wrong mailing list in Cc, adding back linux-wireless ]
Pavel Skripkin <[email protected]> writes:
> Hi Hillf,
>
> On 5/22/22 07:15, Hillf Danton wrote:
>>
>> In the call chain below
>>
>> ath9k_hif_usb_firmware_cb()
>> ath9k_htc_hw_alloc()
>> ath9k_hif_usb_dev_init()
>> ret = ath9k_htc_hw_init()
>> ath9k_htc_probe_device()
>> htc_handle->drv_priv = priv;
>> ret = ath9k_htc_wait_for_target(priv);
>> if (ret)
>> goto err_free;
>> if (ret)
>> goto err_htc_hw_init;
>>
>> err_free:
>> ieee80211_free_hw(hw);
>>
>>
>> err_htc_hw_init:
>> ath9k_hif_usb_dev_deinit(hif_dev);
>> ath9k_hif_usb_dealloc_urbs()
>> err_dev_init:
>> ath9k_htc_hw_free(hif_dev->htc_handle);
>> err_dev_alloc:
>> release_firmware(fw);
>> err_fw:
>> ath9k_hif_usb_firmware_fail(hif_dev);
>>
>>
>> hw should survive deallocating urbs, and changes should be added instead to
>> the rollback in ath9k_htc_probe_device() by deferring cleanup of hw to its
>> callsite in addition to urbs.
>>
>
> Don't get it, sorry. I am not changing the life time of `hw`, I am just
> deferring htc_handle->drv_priv initialization.
>
>
>
>
> With regards,
> Pavel Skripkin
Takashi Iwai <[email protected]> writes:
> On Sat, 21 May 2022 23:28:01 +0200,
> Pavel Skripkin wrote:
>>
>> Syzbot reported use-after-free Read in ath9k_hif_usb_rx_cb() [0]. 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 safe, since syzbot has reported related NULL
>> deref in that macros [1]
>>
>> Link: https://syzkaller.appspot.com/bug?id=6ead44e37afb6866ac0c7dd121b4ce07cb665f60 [0]
>> Link: https://syzkaller.appspot.com/bug?id=b8101ffcec107c0567a0cd8acbbacec91e9ee8de [1]
>> 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]>
>
> Hi Toke, any update on this?
It's marked as "Changes Requested" in patchwork, due to the kernel test
robot comments on patch 2[0]. So it's waiting for Pavel to resubmit
fixing that. There's also a separate comment on patch 1, which I just
noticed didn't have the mailing list in Cc; will reply to that and try
to get it back on the list.
> I'm asking it because this is a fix for a security issue
> (CVE-2022-1679 [*]), and distros have been waiting for the fix getting
> merged to the upstream for weeks.
>
> [*] https://nvd.nist.gov/vuln/detail/CVE-2022-1679
In general, if a patch is marked as "changes requested", the right thing
to do is to bug the submitter to resubmit. Which I guess you just did,
so hopefully we'll get an update soon :)
-Toke
[0] https://patchwork.kernel.org/project/linux-wireless/patch/7bb006bb88e280c596d0e86ece7d251a21b8ed1f.1653168225.git.paskripkin@gmail.com/
Hi Toke,
On 6/10/22 21:30, Toke Høiland-Jørgensen wrote:
>
> In general, if a patch is marked as "changes requested", the right thing
> to do is to bug the submitter to resubmit. Which I guess you just did,
> so hopefully we'll get an update soon :)
>
I agree here. The build fix is trivial, I just wanted to reply to Hillf
like 2 weeks ago, but an email got lost in my inbox.
So, i don't know what is correct thing to do rn: wait for Hillf's reply
or to quickly respin with build error addressed?
With regards,
Pavel Skripkin
Pavel Skripkin <[email protected]> writes:
> Hi Toke,
>
> On 6/10/22 21:30, Toke Høiland-Jørgensen wrote:
>>
>> In general, if a patch is marked as "changes requested", the right thing
>> to do is to bug the submitter to resubmit. Which I guess you just did,
>> so hopefully we'll get an update soon :)
>>
>
>
> I agree here. The build fix is trivial, I just wanted to reply to
> Hillf like 2 weeks ago, but an email got lost in my inbox.
>
> So, i don't know what is correct thing to do rn: wait for Hillf's
> reply or to quickly respin with build error addressed?
Up to you. If you respin it now we can just let it sit in patchwork over
the weekend and see if it attracts any further comment; or you can wait
and respin on Monday...
-Toke