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(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++
#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
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?
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
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
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
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
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(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
With regards,
Pavel Skripkin
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
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(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)
>
> 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
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(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).
-Toke
On 2/7/2022 12:24 PM, Pavel Skripkin wrote:
[...snip...]
>
> #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_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)
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?