Received-SPF: pass (google.com: domain of linux-wireless-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
From:   Kalle Valo <kvalo@kernel.org>
To:     Hillf Danton <hdanton@sina.com>
Cc:     Fedor Pchelkin <pchelkin@ispras.ru>,
        Toke =?utf-8?Q?H=C3=B8iland-J?= =?utf-8?Q?=C3=B8rgensen?= 
        <toke@toke.dk>, linux-kernel@vger.kernel.org,
        syzbot+f2cb6e0ffdb961921e4d@syzkaller.appspotmail.com,
        syzbot+df61b36319e045c00a08@syzkaller.appspotmail.com,
        linux-wireless@vger.kernel.org
Subject: Re: [PATCH v2] wifi: ath9k: fix races between ath9k_wmi_cmd and ath9k_wmi_ctrl_rx
References: <20230425033832.2041-1-hdanton@sina.com>
Date:   Tue, 25 Apr 2023 08:45:06 +0300
In-Reply-To: <20230425033832.2041-1-hdanton@sina.com> (Hillf Danton's message
        of "Tue, 25 Apr 2023 11:38:32 +0800")
Message-ID: <87edo8qzvh.fsf@kernel.org>
User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/26.1 (gnu/linux)
MIME-Version: 1.0
Content-Type: text/plain
Precedence: bulk

Hillf Danton <hdanton@sina.com> writes:

> On 24 Apr 2023 22:18:26 +0300 Fedor Pchelkin <pchelkin@ispras.ru>
>> 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
>
> Without	wmi->wmi_lock held, updating last_seq_id on the waiter side
> means it is random on the waker side, so the fix below is incorrect.
>
>>   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, 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.
>> 
>> 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.
>
> Better in a seperate one.

Adding linux-wireless, please always CC the list with wireless patches.

-- 
https://patchwork.kernel.org/project/linux-wireless/list/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches