From patchwork Tue Apr 25 19:26:06 2023 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Fedor Pchelkin X-Patchwork-Id: 677994 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id D8C66C77B7F for ; Tue, 25 Apr 2023 19:26:36 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S236026AbjDYT0e (ORCPT ); Tue, 25 Apr 2023 15:26:34 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:47340 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S236006AbjDYT0b (ORCPT ); Tue, 25 Apr 2023 15:26:31 -0400 Received: from mail.ispras.ru (mail.ispras.ru [83.149.199.84]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 0B60516181; Tue, 25 Apr 2023 12:26:21 -0700 (PDT) Received: from fpc.intra.ispras.ru (unknown [10.10.165.4]) by mail.ispras.ru (Postfix) with ESMTPSA id AA7D440737A1; Tue, 25 Apr 2023 19:26:16 +0000 (UTC) DKIM-Filter: OpenDKIM Filter v2.11.0 mail.ispras.ru AA7D440737A1 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=ispras.ru; s=default; t=1682450776; bh=vtxoNftItfp9sdIuiR6TC+lugMM08soTNMG6xlHTUHY=; h=From:To:Cc:Subject:Date:In-Reply-To:References:From; b=hHs4+//l7vV8UdP9DwaVLuG8XDv+uZPdvuzCG+8MFti52uZty2gE84aRweqUZIFAQ b9RaIWs3ZkeIvOxVPB2gQ0Jjg2bCUCR5H5xy/LSRYOjFs1v4DeYERpO7JyRMgwnn9D 6nKCZLV/BySqjQd6RgrrvgrIklMSytHd5zxKyGLc= From: Fedor Pchelkin To: =?utf-8?q?Toke_H=C3=B8iland-J=C3=B8rgensen?= , Kalle Vallo Cc: Fedor Pchelkin , "David S. Miller" , Eric Dumazet , Jakub Kicinski , Paolo Abeni , Senthil Balasubramanian , "John W. Linville" , Vasanthakumar Thiagarajan , Sujith , linux-wireless@vger.kernel.org, netdev@vger.kernel.org, linux-kernel@vger.kernel.org, Alexey Khoroshilov , lvc-project@linuxtesting.org, syzbot+f2cb6e0ffdb961921e4d@syzkaller.appspotmail.com, Hillf Danton Subject: [PATCH v3 1/2] wifi: ath9k: fix races between ath9k_wmi_cmd and ath9k_wmi_ctrl_rx Date: Tue, 25 Apr 2023 22:26:06 +0300 Message-Id: <20230425192607.18015-1-pchelkin@ispras.ru> X-Mailer: git-send-email 2.34.1 In-Reply-To: <20230425033832.2041-1-hdanton@sina.com> References: MIME-Version: 1.0 Precedence: bulk List-ID: X-Mailing-List: linux-wireless@vger.kernel.org Currently, the synchronization between ath9k_wmi_cmd() and ath9k_wmi_ctrl_rx() is exposed to a race condition which, although being rather unlikely, can lead to 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 --- /* the callback is being processed * before last_seq_id became zero */ ath9k_wmi_ctrl_rx(...) spin_lock_irqsave(...) /* wmi->last_seq_id check here * doesn't detect timeout yet */ spin_unlock_irqrestore(...) /* last_seq_id is zeroed to * indicate there was a timeout */ wmi->last_seq_id = 0 mutex_unlock(&wmi->op_mutex) return -ETIMEDOUT ath9k_wmi_cmd(...) mutex_lock(&wmi->op_mutex) /* the buffer is replaced with * another one */ wmi->cmd_rsp_buf = rsp_buf wmi->cmd_rsp_len = rsp_len ath9k_wmi_cmd_issue(...) spin_lock_irqsave(...) spin_unlock_irqrestore(...) wait_for_completion_timeout(...) /* the continuation of the * callback left after the first * ath9k_wmi_cmd call */ ath9k_wmi_rsp_callback(...) /* copying data designated * to already timeouted * WMI command into an * inappropriate wmi_cmd_buf */ memcpy(...) complete(&wmi->cmd_wait) /* awakened by the bogus callback * => invalid return result */ mutex_unlock(&wmi->op_mutex) return 0 To fix this, update last_seq_id on timeout path inside ath9k_wmi_cmd() under the wmi_lock. Move 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. Found by Linux Verification Center (linuxtesting.org) with Syzkaller. Fixes: fb9987d0f748 ("ath9k_htc: Support for AR9271 chipset.") Signed-off-by: Fedor Pchelkin Acked-by: Toke Høiland-Jørgensen --- 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 | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/drivers/net/wireless/ath/ath9k/wmi.c b/drivers/net/wireless/ath/ath9k/wmi.c index d652c647d56b..04f363cb90fe 100644 --- a/drivers/net/wireless/ath/ath9k/wmi.c +++ b/drivers/net/wireless/ath/ath9k/wmi.c @@ -242,10 +242,10 @@ static void ath9k_wmi_ctrl_rx(void *priv, struct sk_buff *skb, 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); + spin_unlock_irqrestore(&wmi->wmi_lock, flags); free_skb: kfree_skb(skb); @@ -308,8 +308,8 @@ int ath9k_wmi_cmd(struct wmi *wmi, enum wmi_cmd_id cmd_id, struct ath_common *common = ath9k_hw_common(ah); u16 headroom = sizeof(struct htc_frame_hdr) + sizeof(struct wmi_cmd_hdr); + unsigned long time_left, flags; struct sk_buff *skb; - unsigned long time_left; int ret = 0; if (ah->ah_flags & AH_UNPLUGGED) @@ -345,7 +345,9 @@ int ath9k_wmi_cmd(struct wmi *wmi, enum wmi_cmd_id cmd_id, if (!time_left) { ath_dbg(common, WMI, "Timeout waiting for WMI command: %s\n", wmi_cmd_to_name(cmd_id)); + spin_lock_irqsave(&wmi->wmi_lock, flags); wmi->last_seq_id = 0; + spin_unlock_irqrestore(&wmi->wmi_lock, flags); mutex_unlock(&wmi->op_mutex); return -ETIMEDOUT; }