From: Pavel Skripkin <hidden> Date: 2022-02-07 20:28:33
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: syzbot+03110230a11411024147@syzkaller.appspotmail.com
Reported-and-tested-by: syzbot+c6dde1f690b60e0b9fbe@syzkaller.appspotmail.com
Signed-off-by: Pavel Skripkin <redacted>
---
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(-)
From: Pavel Skripkin <hidden> Date: 2022-02-07 20:27:34
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 <redacted>
---
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(-)
From: Jeff Johnson <hidden> Date: 2022-02-07 21:26:52
On 2/7/2022 12:24 PM, Pavel Skripkin wrote:
quoted hunk
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 <redacted>
---
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(-)
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"?
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
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 <redacted>
---
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(-)
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
From: Pavel Skripkin <hidden> Date: 2022-02-08 15:48:38
Hi Toke,
On 2/8/22 17:47, Toke Høiland-Jørgensen wrote:
Pavel Skripkin [off-list ref] writes:
quoted
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
From: Pavel Skripkin <hidden> Date: 2022-02-08 15:49:38
Hi Toke,
On 2/8/22 18:32, Toke Høiland-Jørgensen wrote:
quoted
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
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?
From: Pavel Skripkin <hidden> Date: 2022-05-05 19:09:55
Hi Tetsuo,
On 5/2/22 09:10, Tetsuo Handa wrote:
quoted
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
From: Pavel Skripkin <hidden> Date: 2022-05-10 19:26:57
Hi Tetsuo,
On 5/6/22 02:31, Tetsuo Handa wrote:
On 2022/05/06 4:09, Pavel Skripkin wrote:
quoted
quoted
quoted
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
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: syzbot+03110230a11411024147@syzkaller.appspotmail.com
Reported-and-tested-by: syzbot+c6dde1f690b60e0b9fbe@syzkaller.appspotmail.com
Signed-off-by: Pavel Skripkin <redacted>
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?
quoted hunk
---
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(-)
From: Pavel Skripkin <hidden> Date: 2022-05-12 12:56:25
Hi Toke,
On 5/12/22 15:49, Toke Høiland-Jørgensen wrote:
Pavel Skripkin [off-list ref] writes:
quoted
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: syzbot+03110230a11411024147@syzkaller.appspotmail.com
Reported-and-tested-by: syzbot+c6dde1f690b60e0b9fbe@syzkaller.appspotmail.com
Signed-off-by: Pavel Skripkin <redacted>
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?
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
quoted
---
[snip]
quoted
-#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
Hi Toke,
On 5/12/22 15:49, Toke Høiland-Jørgensen wrote:
quoted
Pavel Skripkin [off-list ref] writes:
quoted
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: syzbot+03110230a11411024147@syzkaller.appspotmail.com
Reported-and-tested-by: syzbot+c6dde1f690b60e0b9fbe@syzkaller.appspotmail.com
Signed-off-by: Pavel Skripkin <redacted>
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?
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]
quoted
quoted
-#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
From: Jeff Johnson <hidden> Date: 2022-05-12 16:05:33
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?
From: Pavel Skripkin <hidden> Date: 2022-05-12 16:09:33
Hi Jeff,
On 5/12/22 19:05, Jeff Johnson wrote:
On 2/7/2022 12:24 PM, Pavel Skripkin wrote:
[...snip...]
quoted
#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