Syzkaller reports two cases ([1] and [2]) of uninitialized memory referencing in ath9k
wmi functions. The following patch series is intended to fix them and related issues.
[1] https://syzkaller.appspot.com/bug?id=51d401326d8ee41859d68997acdd6f3b1b39f186
[2] https://syzkaller.appspot.com/bug?id=fc54e8d79f5d5082c7867259d71b4e6618b69d25
For the reasons described in commit b383e8abed41 ("wifi: ath9k: avoid
uninit memory read in ath9k_htc_rx_msg()"), ath9k_htc_rx_msg() should
validate pkt_len before accessing the SKB. For example, the obtained SKB
may have uninitialized memory in the case of
ioctl(USB_RAW_IOCTL_EP_WRITE).
Implement sanity checking inside the corresponding endpoint RX handlers:
ath9k_wmi_ctrl_rx() and ath9k_htc_rxep(). Otherwise, uninit memory can
be referenced.
Add comments briefly describing the issue.
Found by Linux Verification Center (linuxtesting.org) with Syzkaller.
Fixes: fb9987d0f748 ("ath9k_htc: Support for AR9271 chipset.")
Reported-and-tested-by: [email protected]
Signed-off-by: Fedor Pchelkin <[email protected]>
---
drivers/net/wireless/ath/ath9k/htc_drv_txrx.c | 6 ++++++
drivers/net/wireless/ath/ath9k/htc_hst.c | 4 ++++
drivers/net/wireless/ath/ath9k/wmi.c | 8 ++++++++
3 files changed, 18 insertions(+)
diff --git a/drivers/net/wireless/ath/ath9k/htc_drv_txrx.c b/drivers/net/wireless/ath/ath9k/htc_drv_txrx.c
index 672789e3c55d..957efb26019d 100644
--- a/drivers/net/wireless/ath/ath9k/htc_drv_txrx.c
+++ b/drivers/net/wireless/ath/ath9k/htc_drv_txrx.c
@@ -1147,6 +1147,12 @@ void ath9k_htc_rxep(void *drv_priv, struct sk_buff *skb,
if (!data_race(priv->rx.initialized))
goto err;
+ /* Validate the obtained SKB so that it is handled without error
+ * inside rx_tasklet handler.
+ */
+ if (unlikely(skb->len < sizeof(struct ieee80211_hdr)))
+ goto err;
+
spin_lock_irqsave(&priv->rx.rxbuflock, flags);
list_for_each_entry(tmp_buf, &priv->rx.rxbuf, list) {
if (!tmp_buf->in_process) {
diff --git a/drivers/net/wireless/ath/ath9k/htc_hst.c b/drivers/net/wireless/ath/ath9k/htc_hst.c
index fe62ff668f75..9d0d9d0e1aa8 100644
--- a/drivers/net/wireless/ath/ath9k/htc_hst.c
+++ b/drivers/net/wireless/ath/ath9k/htc_hst.c
@@ -475,6 +475,10 @@ void ath9k_htc_rx_msg(struct htc_target *htc_handle,
skb_pull(skb, sizeof(struct htc_frame_hdr));
endpoint = &htc_handle->endpoint[epid];
+
+ /* The endpoint RX handlers should implement their own
+ * additional SKB sanity checking
+ */
if (endpoint->ep_callbacks.rx)
endpoint->ep_callbacks.rx(endpoint->ep_callbacks.priv,
skb, epid);
diff --git a/drivers/net/wireless/ath/ath9k/wmi.c b/drivers/net/wireless/ath/ath9k/wmi.c
index 19345b8f7bfd..2e7c361b62f5 100644
--- a/drivers/net/wireless/ath/ath9k/wmi.c
+++ b/drivers/net/wireless/ath/ath9k/wmi.c
@@ -204,6 +204,10 @@ static void ath9k_wmi_rsp_callback(struct wmi *wmi, struct sk_buff *skb)
{
skb_pull(skb, sizeof(struct wmi_cmd_hdr));
+ /* Once again validate the SKB. */
+ if (unlikely(skb->len < wmi->cmd_rsp_len))
+ return;
+
if (wmi->cmd_rsp_buf != NULL && wmi->cmd_rsp_len != 0)
memcpy(wmi->cmd_rsp_buf, skb->data, wmi->cmd_rsp_len);
@@ -221,6 +225,10 @@ static void ath9k_wmi_ctrl_rx(void *priv, struct sk_buff *skb,
if (unlikely(wmi->stopped))
goto free_skb;
+ /* Validate the obtained SKB. */
+ if (unlikely(skb->len < sizeof(struct wmi_cmd_hdr)))
+ goto free_skb;
+
hdr = (struct wmi_cmd_hdr *) skb->data;
cmd_id = be16_to_cpu(hdr->command_id);
--
2.34.1
Currently, the synchronization between ath9k_wmi_cmd() and
ath9k_wmi_ctrl_rx() is exposed to races which can result, for example, not
only in wmi->cmd_rsp_buf being incorrectly initialized, but in overall
invalid behaviour of ath9k_wmi_cmd().
Consider the following scenario:
CPU0 CPU1
ath9k_wmi_cmd(...)
mutex_lock(&wmi->op_mutex)
ath9k_wmi_cmd_issue(...)
wait_for_completion_timeout(...)
---
timeout
---
mutex_unlock(&wmi->op_mutex)
return -ETIMEDOUT
ath9k_wmi_cmd(...)
mutex_lock(&wmi->op_mutex)
ath9k_wmi_cmd_issue(...)
wait_for_completion_timeout(...)
/* the one left after the first
* ath9k_wmi_cmd call
*/
ath9k_wmi_rsp_callback(...)
memcpy(...)
complete(&wmi->cmd_wait);
/* Awakened by the bogus callback
* => invalid return
*/
mutex_unlock(&wmi->op_mutex)
return 0
This may occur even on uniprocessor machines, and in SMP case the races
may be even more intricate.
To fix this, move the contents of ath9k_wmi_rsp_callback() under wmi_lock
inside ath9k_wmi_ctrl_rx() so that the wmi->cmd_wait can be completed only
for initially designated wmi_cmd call, otherwise the path would be
rejected with last_seq_id check.
Also move recording the rsp buffer and length into ath9k_wmi_cmd_issue()
under the same wmi_lock with last_seq_id update to avoid their racy
changes.
Found by Linux Verification Center (linuxtesting.org) with Syzkaller.
Fixes: fb9987d0f748 ("ath9k_htc: Support for AR9271 chipset.")
Reported-and-tested-by: [email protected]
Signed-off-by: Fedor Pchelkin <[email protected]>
---
This patch depends on [PATCH 1/3] wifi: ath9k: avoid referencing uninit
memory in ath9k_wmi_ctrl_rx
drivers/net/wireless/ath/ath9k/wmi.c | 48 ++++++++++++++--------------
1 file changed, 24 insertions(+), 24 deletions(-)
diff --git a/drivers/net/wireless/ath/ath9k/wmi.c b/drivers/net/wireless/ath/ath9k/wmi.c
index 2e7c361b62f5..99a91bbaace9 100644
--- a/drivers/net/wireless/ath/ath9k/wmi.c
+++ b/drivers/net/wireless/ath/ath9k/wmi.c
@@ -200,20 +200,6 @@ void ath9k_fatal_work(struct work_struct *work)
ath9k_htc_reset(priv);
}
-static void ath9k_wmi_rsp_callback(struct wmi *wmi, struct sk_buff *skb)
-{
- skb_pull(skb, sizeof(struct wmi_cmd_hdr));
-
- /* Once again validate the SKB. */
- if (unlikely(skb->len < wmi->cmd_rsp_len))
- return;
-
- if (wmi->cmd_rsp_buf != NULL && wmi->cmd_rsp_len != 0)
- memcpy(wmi->cmd_rsp_buf, skb->data, wmi->cmd_rsp_len);
-
- complete(&wmi->cmd_wait);
-}
-
static void ath9k_wmi_ctrl_rx(void *priv, struct sk_buff *skb,
enum htc_endpoint_id epid)
{
@@ -242,14 +228,26 @@ static void ath9k_wmi_ctrl_rx(void *priv, struct sk_buff *skb,
/* Check if there has been a timeout. */
spin_lock_irqsave(&wmi->wmi_lock, flags);
- if (be16_to_cpu(hdr->seq_no) != wmi->last_seq_id) {
+ if (be16_to_cpu(hdr->seq_no) != wmi->last_seq_id ||
+ be16_to_cpu(hdr->seq_no) == 0) {
+ spin_unlock_irqrestore(&wmi->wmi_lock, flags);
+ goto free_skb;
+ }
+
+ /* Next, process WMI command response */
+ skb_pull(skb, sizeof(struct wmi_cmd_hdr));
+
+ /* Once again validate the SKB. */
+ if (unlikely(skb->len < wmi->cmd_rsp_len)) {
spin_unlock_irqrestore(&wmi->wmi_lock, flags);
goto free_skb;
}
- spin_unlock_irqrestore(&wmi->wmi_lock, flags);
- /* WMI command response */
- ath9k_wmi_rsp_callback(wmi, skb);
+ if (wmi->cmd_rsp_buf != NULL && wmi->cmd_rsp_len != 0)
+ memcpy(wmi->cmd_rsp_buf, skb->data, wmi->cmd_rsp_len);
+
+ complete(&wmi->cmd_wait);
+ spin_unlock_irqrestore(&wmi->wmi_lock, flags);
free_skb:
kfree_skb(skb);
@@ -287,7 +285,8 @@ int ath9k_wmi_connect(struct htc_target *htc, struct wmi *wmi,
static int ath9k_wmi_cmd_issue(struct wmi *wmi,
struct sk_buff *skb,
- enum wmi_cmd_id cmd, u16 len)
+ enum wmi_cmd_id cmd, u16 len,
+ u8 *rsp_buf, u32 rsp_len)
{
struct wmi_cmd_hdr *hdr;
unsigned long flags;
@@ -297,6 +296,11 @@ static int ath9k_wmi_cmd_issue(struct wmi *wmi,
hdr->seq_no = cpu_to_be16(++wmi->tx_seq_id);
spin_lock_irqsave(&wmi->wmi_lock, flags);
+
+ /* record the rsp buffer and length */
+ wmi->cmd_rsp_buf = rsp_buf;
+ wmi->cmd_rsp_len = rsp_len;
+
wmi->last_seq_id = wmi->tx_seq_id;
spin_unlock_irqrestore(&wmi->wmi_lock, flags);
@@ -337,11 +341,7 @@ int ath9k_wmi_cmd(struct wmi *wmi, enum wmi_cmd_id cmd_id,
goto out;
}
- /* record the rsp buffer and length */
- wmi->cmd_rsp_buf = rsp_buf;
- wmi->cmd_rsp_len = rsp_len;
-
- ret = ath9k_wmi_cmd_issue(wmi, skb, cmd_id, cmd_len);
+ ret = ath9k_wmi_cmd_issue(wmi, skb, cmd_id, cmd_len, rsp_buf, rsp_len);
if (ret)
goto out;
--
2.34.1
Return with an error code in case the USB device has been already
unplugged. Otherwise the callers of ath9k_wmi_cmd() are unaware of the
fact that cmd_buf and rsp_buf are not initialized or handled properly
inside this function.
Found by Linux Verification Center (linuxtesting.org) with Syzkaller.
Fixes: a3be14b76da1 ("ath9k_htc: Handle device unplug properly")
Signed-off-by: Fedor Pchelkin <[email protected]>
---
drivers/net/wireless/ath/ath9k/wmi.c | 7 +++++--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/drivers/net/wireless/ath/ath9k/wmi.c b/drivers/net/wireless/ath/ath9k/wmi.c
index 99a91bbaace9..3e0ad4f8f0a0 100644
--- a/drivers/net/wireless/ath/ath9k/wmi.c
+++ b/drivers/net/wireless/ath/ath9k/wmi.c
@@ -320,8 +320,11 @@ int ath9k_wmi_cmd(struct wmi *wmi, enum wmi_cmd_id cmd_id,
unsigned long time_left;
int ret = 0;
- if (ah->ah_flags & AH_UNPLUGGED)
- return 0;
+ if (ah->ah_flags & AH_UNPLUGGED) {
+ ath_dbg(common, WMI, "Device unplugged for WMI command: %s\n",
+ wmi_cmd_to_name(cmd_id));
+ return -ENODEV;
+ }
skb = alloc_skb(headroom + cmd_len, GFP_ATOMIC);
if (!skb)
--
2.34.1
On Wed, Mar 15, 2023 at 11:21:09PM +0300, Fedor Pchelkin wrote:
> Syzkaller reports two cases ([1] and [2]) of uninitialized memory referencing in ath9k
> wmi functions. The following patch series is intended to fix them and related issues.
>
> [1] https://syzkaller.appspot.com/bug?id=51d401326d8ee41859d68997acdd6f3b1b39f186
> [2] https://syzkaller.appspot.com/bug?id=fc54e8d79f5d5082c7867259d71b4e6618b69d25
During the patch development I observed that the return value of REG_READ
(ath9k_regread), REG_READ_MULTI (ath9k_multi_regread) and similar macros
is not checked in most places inside ath9k where they are called. That may
also potentially lead to incorrect behaviour. I wonder if it actually
poses a problem as the current implementation has been for a long time and
perhaps somebody has already addressed this.
In more details:
-- ath9k_regread returns -1 on error, and probably this is a predefined
error state and doesn't need additional check. But, overall, it seems
strange to me that the return value is not checked in places where it
is used later (for example, in ath9k_reg_rmw or
ath9k_hw_ani_read_counters).
-- ath9k_multi_regread fills 'val' buffer with undefined values on error
case, that should definitely be fixed with initializing the local
buffer to zero, I think.
Could you please say your opinion on this issue?
Fedor Pchelkin <[email protected]> writes:
> For the reasons described in commit b383e8abed41 ("wifi: ath9k: avoid
> uninit memory read in ath9k_htc_rx_msg()"), ath9k_htc_rx_msg() should
> validate pkt_len before accessing the SKB. For example, the obtained SKB
> may have uninitialized memory in the case of
> ioctl(USB_RAW_IOCTL_EP_WRITE).
>
> Implement sanity checking inside the corresponding endpoint RX handlers:
> ath9k_wmi_ctrl_rx() and ath9k_htc_rxep(). Otherwise, uninit memory can
> be referenced.
>
> Add comments briefly describing the issue.
>
> Found by Linux Verification Center (linuxtesting.org) with Syzkaller.
It would be also nice to know how you have tested these. Syzkaller is no
substitute for testing on a real hardware.
--
https://patchwork.kernel.org/project/linux-wireless/list/
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
On Fri, Mar 17, 2023 at 07:26:11AM +0200, Kalle Valo wrote:
>
> It would be also nice to know how you have tested these. Syzkaller is no
> substitute for testing on a real hardware.
>
Unfortunately, currently I can't test this on real hardware so probably we
should postpone the patch discussion for some time. Roughly in a week or
two I'll be able to do some testing and try to reproduce the problem
there.
For sure this should be tested on real hardware as some issues may arise.
I sent the patch based on the commit b383e8abed41 ("wifi: ath9k: avoid
uninit memory read in ath9k_htc_rx_msg()") where it is explained
thoroughly what can lead to such behaviour. At the moment I don't see
anything in the code which can prevent that invalid scenario to happen for
endpoint callbacks path.
Actually, sanity checks for SKB length have been added everywhere inside
ath9k_htc_rx_msg() except where the endpoint callbacks are called. As for
the repro, the SKB inside ath9k_hif_usb_rx_stream() is allocated with
pkt_len=8 so it passes the 'htc_frame_hdr' check and processing in
ath9k_htc_rx_msg() but it obviously cannot be handled correctly in the
endpoint callbacks then.
On Wed, Mar 15, 2023 at 11:21:10PM +0300, Fedor Pchelkin wrote:
> For the reasons described in commit b383e8abed41 ("wifi: ath9k: avoid
> uninit memory read in ath9k_htc_rx_msg()"), ath9k_htc_rx_msg() should
> validate pkt_len before accessing the SKB. For example, the obtained SKB
> may have uninitialized memory in the case of
> ioctl(USB_RAW_IOCTL_EP_WRITE).
>
> Implement sanity checking inside the corresponding endpoint RX handlers:
> ath9k_wmi_ctrl_rx() and ath9k_htc_rxep(). Otherwise, uninit memory can
> be referenced.
>
> Add comments briefly describing the issue.
>
> Found by Linux Verification Center (linuxtesting.org) with Syzkaller.
>
> Fixes: fb9987d0f748 ("ath9k_htc: Support for AR9271 chipset.")
> Reported-and-tested-by: [email protected]
> Signed-off-by: Fedor Pchelkin <[email protected]>
> ---
Apologies for the delay.
I've been able to test the patches in some way on 0cf3:9271 Qualcomm
Atheros Communications AR9271 802.11n .
> drivers/net/wireless/ath/ath9k/htc_drv_txrx.c | 6 ++++++
> drivers/net/wireless/ath/ath9k/htc_hst.c | 4 ++++
> drivers/net/wireless/ath/ath9k/wmi.c | 8 ++++++++
> 3 files changed, 18 insertions(+)
>
> diff --git a/drivers/net/wireless/ath/ath9k/htc_drv_txrx.c b/drivers/net/wireless/ath/ath9k/htc_drv_txrx.c
> index 672789e3c55d..957efb26019d 100644
> --- a/drivers/net/wireless/ath/ath9k/htc_drv_txrx.c
> +++ b/drivers/net/wireless/ath/ath9k/htc_drv_txrx.c
> @@ -1147,6 +1147,12 @@ void ath9k_htc_rxep(void *drv_priv, struct sk_buff *skb,
> if (!data_race(priv->rx.initialized))
> goto err;
>
> + /* Validate the obtained SKB so that it is handled without error
> + * inside rx_tasklet handler.
> + */
> + if (unlikely(skb->len < sizeof(struct ieee80211_hdr)))
> + goto err;
> +
> spin_lock_irqsave(&priv->rx.rxbuflock, flags);
> list_for_each_entry(tmp_buf, &priv->rx.rxbuf, list) {
> if (!tmp_buf->in_process) {
This check above seems to be redundant: SKB is properly checked inside
ath9k_rx_prepare() in rx_tasklet handler.
> diff --git a/drivers/net/wireless/ath/ath9k/htc_hst.c b/drivers/net/wireless/ath/ath9k/htc_hst.c
> index fe62ff668f75..9d0d9d0e1aa8 100644
> --- a/drivers/net/wireless/ath/ath9k/htc_hst.c
> +++ b/drivers/net/wireless/ath/ath9k/htc_hst.c
> @@ -475,6 +475,10 @@ void ath9k_htc_rx_msg(struct htc_target *htc_handle,
> skb_pull(skb, sizeof(struct htc_frame_hdr));
>
> endpoint = &htc_handle->endpoint[epid];
> +
> + /* The endpoint RX handlers should implement their own
> + * additional SKB sanity checking
> + */
> if (endpoint->ep_callbacks.rx)
> endpoint->ep_callbacks.rx(endpoint->ep_callbacks.priv,
> skb, epid);
> diff --git a/drivers/net/wireless/ath/ath9k/wmi.c b/drivers/net/wireless/ath/ath9k/wmi.c
> index 19345b8f7bfd..2e7c361b62f5 100644
> --- a/drivers/net/wireless/ath/ath9k/wmi.c
> +++ b/drivers/net/wireless/ath/ath9k/wmi.c
> @@ -204,6 +204,10 @@ static void ath9k_wmi_rsp_callback(struct wmi *wmi, struct sk_buff *skb)
> {
> skb_pull(skb, sizeof(struct wmi_cmd_hdr));
>
> + /* Once again validate the SKB. */
> + if (unlikely(skb->len < wmi->cmd_rsp_len))
> + return;
> +
> if (wmi->cmd_rsp_buf != NULL && wmi->cmd_rsp_len != 0)
> memcpy(wmi->cmd_rsp_buf, skb->data, wmi->cmd_rsp_len);
>
The change above is very very wrong. Looking at the firmware code and
debugging driver with real device, I realized the command response is
located in the tailroom of an SKB: skb->len (SKB data length) is zero in
such packets while skb->data and skb->tail pointers are equal. So a new
skb->len and cmd_rsp_len check resulted in driver denying all WMI command
responses. My bad for proposing such a mistake.
> @@ -221,6 +225,10 @@ static void ath9k_wmi_ctrl_rx(void *priv, struct sk_buff *skb,
> if (unlikely(wmi->stopped))
> goto free_skb;
>
> + /* Validate the obtained SKB. */
> + if (unlikely(skb->len < sizeof(struct wmi_cmd_hdr)))
> + goto free_skb;
> +
> hdr = (struct wmi_cmd_hdr *) skb->data;
> cmd_id = be16_to_cpu(hdr->command_id);
>
This check above is actually good. A packet can be obtained constructed
something like this (taken from Syzkaller reproducer):
*(uint16_t*)0x20000500 = 8; // pkt_len
*(uint16_t*)0x20000502 = 0x4e00; // ATH_USB_RX_STREAM_MODE_TAG
memcpy((void*)0x20000504, "\x15\xa7\xd5\x61\xb9\xb3\xb0\x7c", 8);
syz_usb_ep_write(r[0], 0x82, 0xc, 0x20000500);
pkt_len is 8, so that it can contain an htc_frame_hdr, but when the SKB is
processed in ath9k_htc_rx_msg() and passed to the endpoint RX handler,
ath9k_wmi_ctrl_rx() specifically, the problem arises as there are no other
corresponding headers in the buffer.
wmi_cmd_hdr is pulled later so there are no problems with checking
skb->len. There are no issues arised with the patch on a real device, too.
Not sure if this can happen on an ath9k device with common firmware
(probably can't happen), but that is real with some bad USB device which
Syzkaller emulates.
I'll send the v2 versions for further discussions.
> --
> 2.34.1
>
For the reasons also described in commit b383e8abed41 ("wifi: ath9k: avoid
uninit memory read in ath9k_htc_rx_msg()"), ath9k_htc_rx_msg() should
validate pkt_len before accessing the SKB.
For example, the obtained SKB may have been badly constructed with
pkt_len = 8. In this case, the SKB can only contain a valid htc_frame_hdr
but after being processed in ath9k_htc_rx_msg() and passed to
ath9k_wmi_ctrl_rx() endpoint RX handler, it is expected to have a WMI
command header which should be located inside its data payload.
Implement sanity checking inside ath9k_wmi_ctrl_rx(). Otherwise, uninit
memory can be referenced.
Tested on Qualcomm Atheros Communications AR9271 802.11n .
Found by Linux Verification Center (linuxtesting.org) with Syzkaller.
Fixes: fb9987d0f748 ("ath9k_htc: Support for AR9271 chipset.")
Reported-and-tested-by: [email protected]
Signed-off-by: Fedor Pchelkin <[email protected]>
---
v2: remove incorrect checks and rephrase commit info
drivers/net/wireless/ath/ath9k/wmi.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/drivers/net/wireless/ath/ath9k/wmi.c b/drivers/net/wireless/ath/ath9k/wmi.c
index 19345b8f7bfd..d652c647d56b 100644
--- a/drivers/net/wireless/ath/ath9k/wmi.c
+++ b/drivers/net/wireless/ath/ath9k/wmi.c
@@ -221,6 +221,10 @@ static void ath9k_wmi_ctrl_rx(void *priv, struct sk_buff *skb,
if (unlikely(wmi->stopped))
goto free_skb;
+ /* Validate the obtained SKB. */
+ if (unlikely(skb->len < sizeof(struct wmi_cmd_hdr)))
+ goto free_skb;
+
hdr = (struct wmi_cmd_hdr *) skb->data;
cmd_id = be16_to_cpu(hdr->command_id);
--
2.34.1
This problem is realy subtle, I suppose. In the v2 commit info, which I'll
send in the next mail, the race condition is described which can lead to
invalid behaviour.
Couldn't reproduce that particular problem on real hardware, but if
force timeouts to wmi cmd completions, local KMSan catches some uninit
values.
The synchronization between ath9k_wmi_cmd and ath9k_wmi_ctrl_rx on
timeouts is good, especially after 8a2f35b98306 ("wifi: ath9k: Fix
potential stack-out-of-bounds write in ath9k_wmi_rsp_callback()").
And I think the only place where the fuzzer can provoke failure is when
wmi->last_seq_id in callback is checked before it is assigned zero inside
ath9k_wmi_cmd() during timeout exit. This scenario is more thoroughly
described in patch v2.
Well, the issue seems to be rare and I don't know how to properly test it
on real hardware.
I've made some checks on a basic driver workflow, and there weren't any
stalls or explicit failures, and the patch seems to close that tiny race
condition window. But, anyway, it requires more discussion.