2023-04-25 19:26:44

by Fedor Pchelkin

Subject: [PATCH v3 2/2] wifi: ath9k: protect WMI command response buffer replacement with a lock

If ath9k_wmi_cmd() has exited with a timeout, it is possible that during
next ath9k_wmi_cmd() call the wmi_rsp callback for previous wmi command
writes to new wmi->cmd_rsp_buf and makes a completion. This results in an
invalid ath9k_wmi_cmd() return value.

Move the replacement of WMI command response buffer and length under
wmi_lock. Note that last_seq_id value is updated there, too.

Thus, the buffer cannot be written to by a belated wmi_rsp callback
because that path is properly rejected by the last_seq_id check.

Found by Linux Verification Center (linuxtesting.org) with Syzkaller.

Fixes: fb9987d0f748 ("ath9k_htc: Support for AR9271 chipset.")
Signed-off-by: Fedor Pchelkin <[email protected]>
---
v2: do not extract ath9k_wmi_rsp_callback() internals, rephrase
description
v3: per Hillf Danton's comment, protect last_seq_id with wmi_lock on
timeout path, divide the v2 version into two separate patches; the first
one is concerned with last_seq_id and completion under wmi_lock, the
second one is for moving rsp buffer and length recording under wmi lock.
Note that it's been only tested with Syzkaller and on basic driver
scenarios with real hardware.

drivers/net/wireless/ath/ath9k/wmi.c | 14 ++++++++------
1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/drivers/net/wireless/ath/ath9k/wmi.c b/drivers/net/wireless/ath/ath9k/wmi.c
index 04f363cb90fe..1476b42b52a9 100644
--- a/drivers/net/wireless/ath/ath9k/wmi.c
+++ b/drivers/net/wireless/ath/ath9k/wmi.c
@@ -283,7 +283,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;
@@ -293,6 +294,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);

@@ -333,11 +339,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

2023-08-08 18:24:58

by Toke Høiland-Jørgensen

Subject: Re: [PATCH v3 2/2] wifi: ath9k: protect WMI command response buffer replacement with a lock

Fedor Pchelkin <[email protected]> writes:

> If ath9k_wmi_cmd() has exited with a timeout, it is possible that during
> next ath9k_wmi_cmd() call the wmi_rsp callback for previous wmi command
> writes to new wmi->cmd_rsp_buf and makes a completion. This results in an
> invalid ath9k_wmi_cmd() return value.
>
> Move the replacement of WMI command response buffer and length under
> wmi_lock. Note that last_seq_id value is updated there, too.
>
> Thus, the buffer cannot be written to by a belated wmi_rsp callback
> because that path is properly rejected by the last_seq_id check.
>
> Found by Linux Verification Center (linuxtesting.org) with Syzkaller.
>
> Fixes: fb9987d0f748 ("ath9k_htc: Support for AR9271 chipset.")
> Signed-off-by: Fedor Pchelkin <[email protected]>

Given that the previous patch resets the last_seq_id to 0 on timeout
under the lock, I don't think this patch is strictly necessary anymore.
However, it doesn't hurt either, and I actually think moving the update
of the rsp buf into ath9k_wmi_cmd_issue() aids readability, so:

Acked-by: Toke Høiland-Jørgensen <[email protected]>