Re: [PATCH v3 1/2] ath9k: fix use-after-free in ath9k_hif_usb_rx_cb

Pavel Skripkin [off-list ref] writes:

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,

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!

quoted

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