| Message ID | ZFXk/AIKeapT72Pj@DESKTOP (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Toke Høiland-Jørgensen |
| Headers | show |
| Series | wifi: ath9k: Fix memory leak in htc_connect_service | expand |
Takeshi Misawa <jeantsuru.cumc.mandola@gmail.com> writes: > Timeout occurs in htc_connect_service(), then this function returns > without freeing skb. > > Fix this by going to err path. > > syzbot report: > BUG: memory leak > unreferenced object 0xffff88810a980800 (size 240): > comm "kworker/1:1", pid 24, jiffies 4294947427 (age 16.220s) > hex dump (first 32 bytes): > 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................ > 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................ > backtrace: > [<ffffffff83b971c6>] __alloc_skb+0x206/0x270 net/core/skbuff.c:552 > [<ffffffff82eb3731>] alloc_skb include/linux/skbuff.h:1270 [inline] > [<ffffffff82eb3731>] htc_connect_service+0x121/0x230 drivers/net/wireless/ath/ath9k/htc_hst.c:259 > [<ffffffff82ec03a5>] ath9k_htc_connect_svc drivers/net/wireless/ath/ath9k/htc_drv_init.c:137 [inline] > [<ffffffff82ec03a5>] ath9k_init_htc_services.constprop.0+0xe5/0x390 drivers/net/wireless/ath/ath9k/htc_drv_init.c:157 > [<ffffffff82ec0747>] ath9k_htc_probe_device+0xf7/0x8a0 drivers/net/wireless/ath/ath9k/htc_drv_init.c:959 > [<ffffffff82eb3ef5>] ath9k_htc_hw_init+0x35/0x60 drivers/net/wireless/ath/ath9k/htc_hst.c:521 > [<ffffffff82eb68dd>] ath9k_hif_usb_firmware_cb+0xcd/0x1f0 drivers/net/wireless/ath/ath9k/hif_usb.c:1243 > [<ffffffff82aa835b>] request_firmware_work_func+0x4b/0x90 drivers/base/firmware_loader/main.c:1107 > [<ffffffff8129a35a>] process_one_work+0x2ba/0x5f0 kernel/workqueue.c:2289 > [<ffffffff8129ac7d>] worker_thread+0x5d/0x5b0 kernel/workqueue.c:2436 > [<ffffffff812a4fa9>] kthread+0x129/0x170 kernel/kthread.c:376 > [<ffffffff81002dcf>] ret_from_fork+0x1f/0x30 arch/x86/entry/entry_64.S:308 > > Fixes: fb9987d0f748 ("ath9k_htc: Support for AR9271 chipset.") > Link: https://syzkaller.appspot.com/bug?id=fbf138952d6c1115ba7d797cf7d56f6935184e3f > Reported-and-tested-by: syzbot+b68fbebe56d8362907e8@syzkaller.appspotmail.com > Signed-off-by: Takeshi Misawa <jeantsuru.cumc.mandola@gmail.com> Then submitting a new version of the patch you should mark it as v2 and include a changelog what changed from v1, more info in the wiki link below. But no need to resubmit the patch because of this, just keep this in mind for future patches.
Hi, On Sat, May 06, 2023 at 02:26:20PM +0900, Takeshi Misawa wrote: > Timeout occurs in htc_connect_service(), then this function returns > without freeing skb. > > Fix this by going to err path. This will lead to UAF [1]. If a timeout occurs, htc_connect_service() returns with error but it is possible that a belated callback will touch the already freed SKB. It is actually a callback's responsibilty to free the buffer. The callback is ath9k_htc_txcompletion_cb(). As the urb is submitted, callback should be always executed, otherwise there is an error in usb core, not ath9k. So the problem lies in somewhat another place. htc_connect_service() issues service connection requests via control ENDPOINT0. Its pipe_ids are configured in ath9k_htc_hw_alloc(): /* Assign control endpoint pipe IDs */ endpoint = &target->endpoint[ENDPOINT0]; endpoint->ul_pipeid = hif->control_ul_pipe; endpoint->dl_pipeid = hif->control_dl_pipe; ul_pipe is USB_REG_OUT_PIPE and service connection requests should go that way. However, the reproducer managed to issue the WMI_CMD and Beacon requests via USB_REG_OUT_PIPE, but the third one was issued via USB_WLAN_TX_PIPE. As it went on the wrong way, the SKB simply got lost in __hif_usb_tx() as the special conditions weren't satisfied. Somehow the ENDPOINT0 ul_pipeid has unexpectedly changed... Well, it is htc_process_conn_rsp() which received a badly constructed "service connection reply" from a bad USB device: the endpoint which attributes were to be changed pointed to ENDPOINT0 which should never happen with healthy firmware. But ENDPOINT0 pipeid attributes were changed, so the next service conn requests went on the wrong path. The fix seems to be that htc_process_conn_rsp() should immediately return if the received endpoint which is to be connected to a new service is ENDPOINT0. [1]: https://lore.kernel.org/linux-wireless/20200404041838.10426-2-hqjagain@gmail.com/ > > syzbot report: > BUG: memory leak > unreferenced object 0xffff88810a980800 (size 240): > comm "kworker/1:1", pid 24, jiffies 4294947427 (age 16.220s) > hex dump (first 32 bytes): > 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................ > 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................ > backtrace: > [<ffffffff83b971c6>] __alloc_skb+0x206/0x270 net/core/skbuff.c:552 > [<ffffffff82eb3731>] alloc_skb include/linux/skbuff.h:1270 [inline] > [<ffffffff82eb3731>] htc_connect_service+0x121/0x230 drivers/net/wireless/ath/ath9k/htc_hst.c:259 > [<ffffffff82ec03a5>] ath9k_htc_connect_svc drivers/net/wireless/ath/ath9k/htc_drv_init.c:137 [inline] > [<ffffffff82ec03a5>] ath9k_init_htc_services.constprop.0+0xe5/0x390 drivers/net/wireless/ath/ath9k/htc_drv_init.c:157 > [<ffffffff82ec0747>] ath9k_htc_probe_device+0xf7/0x8a0 drivers/net/wireless/ath/ath9k/htc_drv_init.c:959 > [<ffffffff82eb3ef5>] ath9k_htc_hw_init+0x35/0x60 drivers/net/wireless/ath/ath9k/htc_hst.c:521 > [<ffffffff82eb68dd>] ath9k_hif_usb_firmware_cb+0xcd/0x1f0 drivers/net/wireless/ath/ath9k/hif_usb.c:1243 > [<ffffffff82aa835b>] request_firmware_work_func+0x4b/0x90 drivers/base/firmware_loader/main.c:1107 > [<ffffffff8129a35a>] process_one_work+0x2ba/0x5f0 kernel/workqueue.c:2289 > [<ffffffff8129ac7d>] worker_thread+0x5d/0x5b0 kernel/workqueue.c:2436 > [<ffffffff812a4fa9>] kthread+0x129/0x170 kernel/kthread.c:376 > [<ffffffff81002dcf>] ret_from_fork+0x1f/0x30 arch/x86/entry/entry_64.S:308 > > Fixes: fb9987d0f748 ("ath9k_htc: Support for AR9271 chipset.") > Link: https://syzkaller.appspot.com/bug?id=fbf138952d6c1115ba7d797cf7d56f6935184e3f > Reported-and-tested-by: syzbot+b68fbebe56d8362907e8@syzkaller.appspotmail.com > Signed-off-by: Takeshi Misawa <jeantsuru.cumc.mandola@gmail.com> > --- > drivers/net/wireless/ath/ath9k/htc_hst.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/drivers/net/wireless/ath/ath9k/htc_hst.c b/drivers/net/wireless/ath/ath9k/htc_hst.c > index ca05b07a45e6..6878da6d15b4 100644 > --- a/drivers/net/wireless/ath/ath9k/htc_hst.c > +++ b/drivers/net/wireless/ath/ath9k/htc_hst.c > @@ -285,7 +285,8 @@ int htc_connect_service(struct htc_target *target, > if (!time_left) { > dev_err(target->dev, "Service connection timeout for: %d\n", > service_connreq->service_id); > - return -ETIMEDOUT; > + ret = -ETIMEDOUT; > + goto err; > } > > *conn_rsp_epid = target->conn_rsp_epid; > -- > 2.39.2 >
diff --git a/drivers/net/wireless/ath/ath9k/htc_hst.c b/drivers/net/wireless/ath/ath9k/htc_hst.c index ca05b07a45e6..6878da6d15b4 100644 --- a/drivers/net/wireless/ath/ath9k/htc_hst.c +++ b/drivers/net/wireless/ath/ath9k/htc_hst.c @@ -285,7 +285,8 @@ int htc_connect_service(struct htc_target *target, if (!time_left) { dev_err(target->dev, "Service connection timeout for: %d\n", service_connreq->service_id); - return -ETIMEDOUT; + ret = -ETIMEDOUT; + goto err; } *conn_rsp_epid = target->conn_rsp_epid;
Timeout occurs in htc_connect_service(), then this function returns without freeing skb. Fix this by going to err path. syzbot report: BUG: memory leak unreferenced object 0xffff88810a980800 (size 240): comm "kworker/1:1", pid 24, jiffies 4294947427 (age 16.220s) hex dump (first 32 bytes): 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................ 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................ backtrace: [<ffffffff83b971c6>] __alloc_skb+0x206/0x270 net/core/skbuff.c:552 [<ffffffff82eb3731>] alloc_skb include/linux/skbuff.h:1270 [inline] [<ffffffff82eb3731>] htc_connect_service+0x121/0x230 drivers/net/wireless/ath/ath9k/htc_hst.c:259 [<ffffffff82ec03a5>] ath9k_htc_connect_svc drivers/net/wireless/ath/ath9k/htc_drv_init.c:137 [inline] [<ffffffff82ec03a5>] ath9k_init_htc_services.constprop.0+0xe5/0x390 drivers/net/wireless/ath/ath9k/htc_drv_init.c:157 [<ffffffff82ec0747>] ath9k_htc_probe_device+0xf7/0x8a0 drivers/net/wireless/ath/ath9k/htc_drv_init.c:959 [<ffffffff82eb3ef5>] ath9k_htc_hw_init+0x35/0x60 drivers/net/wireless/ath/ath9k/htc_hst.c:521 [<ffffffff82eb68dd>] ath9k_hif_usb_firmware_cb+0xcd/0x1f0 drivers/net/wireless/ath/ath9k/hif_usb.c:1243 [<ffffffff82aa835b>] request_firmware_work_func+0x4b/0x90 drivers/base/firmware_loader/main.c:1107 [<ffffffff8129a35a>] process_one_work+0x2ba/0x5f0 kernel/workqueue.c:2289 [<ffffffff8129ac7d>] worker_thread+0x5d/0x5b0 kernel/workqueue.c:2436 [<ffffffff812a4fa9>] kthread+0x129/0x170 kernel/kthread.c:376 [<ffffffff81002dcf>] ret_from_fork+0x1f/0x30 arch/x86/entry/entry_64.S:308 Fixes: fb9987d0f748 ("ath9k_htc: Support for AR9271 chipset.") Link: https://syzkaller.appspot.com/bug?id=fbf138952d6c1115ba7d797cf7d56f6935184e3f Reported-and-tested-by: syzbot+b68fbebe56d8362907e8@syzkaller.appspotmail.com Signed-off-by: Takeshi Misawa <jeantsuru.cumc.mandola@gmail.com> --- drivers/net/wireless/ath/ath9k/htc_hst.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)