Received-SPF: pass (google.com: domain of linux-wireless+bounces-5112-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
From: Toke =?utf-8?Q?H=C3=B8iland-J=C3=B8rgensen?= <toke@toke.dk>
To: Edward Adam Davis <eadavis@qq.com>,
 syzbot+93cbd5fbb85814306ba1@syzkaller.appspotmail.com
Cc: kvalo@kernel.org, linux-kernel@vger.kernel.org,
 linux-usb@vger.kernel.org, linux-wireless@vger.kernel.org,
 netdev@vger.kernel.org, syzkaller-bugs@googlegroups.com
Subject: Re: [PATCH usb] wifi: ath9k: fix oob in htc_issue_send
In-Reply-To: <tencent_7225DC0D859205DD8BDDAE191CCFBF0D8907@qq.com>
References: <0000000000004e41110614187d35@google.com>
 <tencent_7225DC0D859205DD8BDDAE191CCFBF0D8907@qq.com>
Date: Thu, 21 Mar 2024 19:52:15 +0100
Message-ID: <87wmpvfme8.fsf@toke.dk>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain

Edward Adam Davis <eadavis@qq.com> writes:

> [syzbot reported]
> usb 1-1: ath9k_htc: Transferred FW: ath9k_htc/htc_9271-1.4.0.fw, size: 51008
> ath9k_htc 1-1:1.0: ath9k_htc: HTC initialized with 33 credits
> ------------[ cut here ]------------
> UBSAN: array-index-out-of-bounds in drivers/net/wireless/ath/ath9k/htc_hst.c:26:51
> index 255 is out of range for type 'htc_endpoint [22]'
> CPU: 1 PID: 2494 Comm: kworker/1:2 Not tainted 6.8.0-rc6-syzkaller-00190-ga788e53c05ae #0
> Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 02/29/2024
> Workqueue: events request_firmware_work_func
> Call Trace:
>  <TASK>
>  __dump_stack lib/dump_stack.c:88 [inline]
>  dump_stack_lvl+0x125/0x1b0 lib/dump_stack.c:106
>  ubsan_epilogue lib/ubsan.c:217 [inline]
>  __ubsan_handle_out_of_bounds+0x111/0x150 lib/ubsan.c:347
>  htc_issue_send.constprop.0+0x209/0x230 drivers/net/wireless/ath/ath9k/htc_hst.c:26
>  ath9k_wmi_cmd_issue drivers/net/wireless/ath/ath9k/wmi.c:305 [inline]
>  ath9k_wmi_cmd+0x424/0x630 drivers/net/wireless/ath/ath9k/wmi.c:342
>  ath9k_regread+0xdb/0x160 drivers/net/wireless/ath/ath9k/htc_drv_init.c:242
>  ath9k_hw_read_revisions drivers/net/wireless/ath/ath9k/hw.c:287 [inline]
>  __ath9k_hw_init drivers/net/wireless/ath/ath9k/hw.c:572 [inline]
>  ath9k_hw_init+0xf02/0x2b30 drivers/net/wireless/ath/ath9k/hw.c:700
>  ath9k_init_priv drivers/net/wireless/ath/ath9k/htc_drv_init.c:662 [inline]
>  ath9k_init_device drivers/net/wireless/ath/ath9k/htc_drv_init.c:839 [inline]
>  ath9k_htc_probe_device+0xb37/0x25f0 drivers/net/wireless/ath/ath9k/htc_drv_init.c:963
>  ath9k_htc_hw_init+0x33/0x70 drivers/net/wireless/ath/ath9k/htc_hst.c:529
>  ath9k_hif_usb_firmware_cb+0x272/0x620 drivers/net/wireless/ath/ath9k/hif_usb.c:1273
>  request_firmware_work_func+0x13a/0x240 drivers/base/firmware_loader/main.c:1163
>  process_one_work+0x886/0x15d0 kernel/workqueue.c:2633
>  process_scheduled_works kernel/workqueue.c:2706 [inline]
>  worker_thread+0x8b9/0x1290 kernel/workqueue.c:2787
>  kthread+0x2c6/0x3a0 kernel/kthread.c:388
>  ret_from_fork+0x45/0x80 arch/x86/kernel/process.c:147
>  ret_from_fork_asm+0x11/0x20 arch/x86/entry/entry_64.S:243
>  </TASK>
> ---[ end trace ]---
> [Fix]
> If the target does not return a valid end point id during the device connection
> process, returns a failure.
>
> Fixes: fb9987d0f748 ("ath9k_htc: Support for AR9271 chipset.")
> Reported-and-tested-by: syzbot+93cbd5fbb85814306ba1@syzkaller.appspotmail.com
> Signed-off-by: Edward Adam Davis <eadavis@qq.com>
> ---
>  drivers/net/wireless/ath/ath9k/htc_hst.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/drivers/net/wireless/ath/ath9k/htc_hst.c b/drivers/net/wireless/ath/ath9k/htc_hst.c
> index eb631fd3336d..0d1115d1cc29 100644
> --- a/drivers/net/wireless/ath/ath9k/htc_hst.c
> +++ b/drivers/net/wireless/ath/ath9k/htc_hst.c
> @@ -295,6 +295,9 @@ int htc_connect_service(struct htc_target *target,
>  	}
>  
>  	*conn_rsp_epid = target->conn_rsp_epid;
> +	if (*conn_rsp_epid < 0 || *conn_rsp_epid > ENDPOINT_MAX)
> +		return -EINVAL;
> +

Hmm, there's an off-by-one error here: the arrays in question are
defined as var[ENDPOINT_MAX], so a value of ENDPOINT_MAX is going to
overflow.

IOW, this should be:

 +	if (*conn_rsp_epid < 0 || *conn_rsp_epid >= ENDPOINT_MAX)
 +		return -EINVAL;
 +


-Toke