[PATCH v2] smb: fix a possible data race in cifs_can_echo()

[PATCH v2] smb: fix a possible data race in cifs_can_echo()

[Tuo Li]

The struct field TCP_Server_Info.tcpStatus is often protected by the lock
srv_lock when is accessed. Here is an example in __cifs_reconnect():

  spin_lock(&server->srv_lock);
  if (server->tcpStatus != CifsExiting)
    server->tcpStatus = CifsNeedNegotiate;
  spin_unlock(&server->srv_lock);

However, the variable server->tcpStatus is accessed without holding the
lock server->srv_lock in cifs_can_echo():

  if (server->tcpStatus == CifsGood)
    return true;

To fix this possible data race, a lock and unlock pair is added when
accessing the variable server->tcpStatus.

Reported-by: BassCheck <bass@buaa.edu.cn>
Signed-off-by: Tuo Li <islituo@gmail.com>
---
v2:
* Release the lock server->srv_lock in the false branch.
---
 fs/smb/client/smb1ops.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/fs/smb/client/smb1ops.c b/fs/smb/client/smb1ops.c
index 7d1b3fc014d9..5120241d3c0e 100644
--- a/fs/smb/client/smb1ops.c
+++ b/fs/smb/client/smb1ops.c
@@ -1049,8 +1049,12 @@ cifs_dir_needs_close(struct cifsFileInfo *cfile)
 static bool
 cifs_can_echo(struct TCP_Server_Info *server)
 {
-	if (server->tcpStatus == CifsGood)
+	spin_lock(&server->srv_lock);
+	if (server->tcpStatus == CifsGood) {
+		spin_unlock(&server->srv_lock);
 		return true;
+	}
+	spin_unlock(&server->srv_lock);
 
 	return false;
 }
-- 
2.34.1

Re: [PATCH v2] smb: fix a possible data race in cifs_can_echo()

[Tom Talpey]

On 6/15/2023 2:38 AM, Tuo Li wrote:
> The struct field TCP_Server_Info.tcpStatus is often protected by the lock
> srv_lock when is accessed. Here is an example in __cifs_reconnect():
> 
>    spin_lock(&server->srv_lock);
>    if (server->tcpStatus != CifsExiting)
>      server->tcpStatus = CifsNeedNegotiate;
>    spin_unlock(&server->srv_lock);
> 
> However, the variable server->tcpStatus is accessed without holding the
> lock server->srv_lock in cifs_can_echo():
> 
>    if (server->tcpStatus == CifsGood)
>      return true;
> 
> To fix this possible data race, a lock and unlock pair is added when
> accessing the variable server->tcpStatus.

This is a NAK from me. The tcpStatus field is not being modified
here, and the race is trivial because the lock you propose is
dropped immediately either way.

Besides, this is SMB1, and the entirely unimportant ECHO procedure.

Tom.


> Reported-by: BassCheck <bass@buaa.edu.cn>
> Signed-off-by: Tuo Li <islituo@gmail.com>
> ---
> v2:
> * Release the lock server->srv_lock in the false branch.
> ---
>   fs/smb/client/smb1ops.c | 6 +++++-
>   1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/smb/client/smb1ops.c b/fs/smb/client/smb1ops.c
> index 7d1b3fc014d9..5120241d3c0e 100644
> --- a/fs/smb/client/smb1ops.c
> +++ b/fs/smb/client/smb1ops.c
> @@ -1049,8 +1049,12 @@ cifs_dir_needs_close(struct cifsFileInfo *cfile)
>   static bool
>   cifs_can_echo(struct TCP_Server_Info *server)
>   {
> -	if (server->tcpStatus == CifsGood)
> +	spin_lock(&server->srv_lock);
> +	if (server->tcpStatus == CifsGood) {
> +		spin_unlock(&server->srv_lock);
>   		return true;
> +	}
> +	spin_unlock(&server->srv_lock);
>   
>   	return false;
>   }