[PATCH] wifi: ath9k: hif_usb: don't dereference hif_dev after re-arming firmware request

[PATCH] wifi: ath9k: hif_usb: don't dereference hif_dev after re-arming firmware request

[xiaoblac]

From: Cheng Yongkang <teel4res@gmail.com>

ath9k_hif_request_firmware() re-arms an asynchronous firmware load via
request_firmware_nowait(), passing hif_dev as the completion context, and
then still dereferences hif_dev:

	dev_info(&hif_dev->udev->dev, "ath9k_htc: Firmware %s requested\n",
		 hif_dev->fw_name);

The re-armed callback ath9k_hif_usb_firmware_cb() runs on the "events"
workqueue and, when the firmware is missing, walks the retry chain into
ath9k_hif_usb_firmware_fail() -> complete_all(&hif_dev->fw_done). That
releases the wait_for_completion(&hif_dev->fw_done) in a concurrent
ath9k_hif_usb_disconnect(), which then kfree()s hif_dev. The trailing
dev_info() in the frame that re-armed the request can therefore read freed
memory (hif_dev->udev, the first field of struct hif_device_usb):

  BUG: KASAN: slab-use-after-free in ath9k_hif_request_firmware
  Read of size 8 ... by task kworker/...
   ath9k_hif_request_firmware
   ath9k_hif_usb_firmware_cb           drivers/net/wireless/ath/ath9k/hif_usb.c:1247
   request_firmware_work_func
  Allocated by ...:
   ath9k_hif_usb_probe                 drivers/net/wireless/ath/ath9k/hif_usb.c
  Freed by ...:
   ath9k_hif_usb_disconnect -> kfree   drivers/net/wireless/ath/ath9k/hif_usb.c

The fw_done barrier only makes disconnect wait for the firmware chain to
*terminate*; it does not protect the outer ath9k_hif_request_firmware()
frame that re-armed the request and keeps touching hif_dev afterwards.

Drop the post-request dev_info(): it is the only use of hif_dev after the
async request is armed, and it is purely informational (the dev_err() on the
failure path runs only when request_firmware_nowait() did not arm a callback,
so hif_dev is still alive there).

This was first reported by syzbot as a single, non-reproduced crash that was
later auto-obsoleted, and was independently rediscovered by the reFuzz fuzzer,
which produced a C reproducer (USB-gadget connect/disconnect of an ath9k_htc
device whose firmware download fails). The vulnerable code is unchanged and
still present in v7.1-rc6, where the slab-use-after-free reproduces under KASAN
once the (sub-microsecond) race window is widened.

Fixes: e904cf6fe230 ("ath9k_htc: introduce support for different fw versions")
Reported-by: syzbot+50122cbc2874b1eb25b0@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=50122cbc2874b1eb25b0
Signed-off-by: Cheng Yongkang <teel4res@gmail.com>
---
 drivers/net/wireless/ath/ath9k/hif_usb.c | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

--- a/drivers/net/wireless/ath/ath9k/hif_usb.c
+++ b/drivers/net/wireless/ath/ath9k/hif_usb.c
@@ -1225,15 +1225,10 @@ static int ath9k_hif_request_firmware(struct hif_device_usb *hif_dev,
 	ret = request_firmware_nowait(THIS_MODULE, true, hif_dev->fw_name,
 				      &hif_dev->udev->dev, GFP_KERNEL,
 				      hif_dev, ath9k_hif_usb_firmware_cb);
-	if (ret) {
+	if (ret)
 		dev_err(&hif_dev->udev->dev,
 			"ath9k_htc: Async request for firmware %s failed\n",
 			hif_dev->fw_name);
-		return ret;
-	}
-
-	dev_info(&hif_dev->udev->dev, "ath9k_htc: Firmware %s requested\n",
-		 hif_dev->fw_name);

 	return ret;
 }

Re: [PATCH] wifi: ath9k: hif_usb: don't dereference hif_dev after re-arming firmware request

[Toke Høiland-Jørgensen]

xiaoblac <teel4res@gmail.com> writes:

> From: Cheng Yongkang <teel4res@gmail.com>
>
> ath9k_hif_request_firmware() re-arms an asynchronous firmware load via
> request_firmware_nowait(), passing hif_dev as the completion context, and
> then still dereferences hif_dev:
>
> 	dev_info(&hif_dev->udev->dev, "ath9k_htc: Firmware %s requested\n",
> 		 hif_dev->fw_name);
>
> The re-armed callback ath9k_hif_usb_firmware_cb() runs on the "events"
> workqueue and, when the firmware is missing, walks the retry chain into
> ath9k_hif_usb_firmware_fail() -> complete_all(&hif_dev->fw_done). That
> releases the wait_for_completion(&hif_dev->fw_done) in a concurrent
> ath9k_hif_usb_disconnect(), which then kfree()s hif_dev. The trailing
> dev_info() in the frame that re-armed the request can therefore read freed
> memory (hif_dev->udev, the first field of struct hif_device_usb):
>
>   BUG: KASAN: slab-use-after-free in ath9k_hif_request_firmware
>   Read of size 8 ... by task kworker/...
>    ath9k_hif_request_firmware
>    ath9k_hif_usb_firmware_cb           drivers/net/wireless/ath/ath9k/hif_usb.c:1247
>    request_firmware_work_func
>   Allocated by ...:
>    ath9k_hif_usb_probe                 drivers/net/wireless/ath/ath9k/hif_usb.c
>   Freed by ...:
>    ath9k_hif_usb_disconnect -> kfree   drivers/net/wireless/ath/ath9k/hif_usb.c
>
> The fw_done barrier only makes disconnect wait for the firmware chain to
> *terminate*; it does not protect the outer ath9k_hif_request_firmware()
> frame that re-armed the request and keeps touching hif_dev afterwards.
>
> Drop the post-request dev_info(): it is the only use of hif_dev after the
> async request is armed, and it is purely informational (the dev_err() on the
> failure path runs only when request_firmware_nowait() did not arm a callback,
> so hif_dev is still alive there).
>
> This was first reported by syzbot as a single, non-reproduced crash that was
> later auto-obsoleted, and was independently rediscovered by the reFuzz fuzzer,
> which produced a C reproducer (USB-gadget connect/disconnect of an ath9k_htc
> device whose firmware download fails). The vulnerable code is unchanged and
> still present in v7.1-rc6, where the slab-use-after-free reproduces under KASAN
> once the (sub-microsecond) race window is widened.
>
> Fixes: e904cf6fe230 ("ath9k_htc: introduce support for different fw versions")
> Reported-by: syzbot+50122cbc2874b1eb25b0@syzkaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?extid=50122cbc2874b1eb25b0
> Signed-off-by: Cheng Yongkang <teel4res@gmail.com>

Thank you for the fix!

Acked-by: Toke Høiland-Jørgensen <toke@toke.dk>