| Message ID | 20230425192607.18015-2-pchelkin@ispras.ru (mailing list archive) |
|---|---|
| State | Awaiting Upstream |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [v3,1/2] wifi: ath9k: fix races between ath9k_wmi_cmd and ath9k_wmi_ctrl_rx | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Not a local patch |
Fedor Pchelkin <pchelkin@ispras.ru> 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 <pchelkin@ispras.ru> 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 <toke@toke.dk>
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;
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 <pchelkin@ispras.ru> --- 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(-)