Re: [PATCH v3 1/2] wifi: ath9k: fix races between ath9k_wmi_cmd and ath9k_wmi_ctrl_rx

[Kalle Valo]

Fedor Pchelkin <pchelkin@xxxxxxxxx> wrote:

> 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 <pchelkin@xxxxxxxxx>
> Acked-by: Toke Høiland-Jørgensen <toke@xxxxxxx>
> Signed-off-by: Kalle Valo <quic_kvalo@xxxxxxxxxxx>

2 patches applied to ath-next branch of ath.git, thanks.

b674fb513e2e wifi: ath9k: fix races between ath9k_wmi_cmd and ath9k_wmi_ctrl_rx
454994cfa9e4 wifi: ath9k: protect WMI command response buffer replacement with a lock

-- 
https://patchwork.kernel.org/project/linux-wireless/patch/20230425192607.18015-1-pchelkin@xxxxxxxxx/

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