Re: [PATCH 2/4] smb: client: fix use-after-free bug in cifs_debug_data_proc_show()

[Wang Zhaolong <wangzhaolong1@huawei.com>]

Hello,

I encountered some confusion while reviewing the source code related to
CVE-2023-52752.

I was able to reproduce the issue, and the original problem seems to be:

---
process 1                   process 2(read /proc/fs/cifs/DebugData)

cifs_umount
cifs_put_tlink
cifs_put_tcon
cifs_put_smb_ses                cifs_debug_data_proc_show
   spin_unlock(&cifs_tcp_ses_lock)
                                   spin_lock(&cifs_tcp_ses_lock);
                                   list_for_each...(ses,server->smb_ses_list,...)
   cifs_free_ipc
     tconInfoFree(tcon)
                                   if (ses->tcon_ipc)
                                    cifs_debug_tcon(m,ses->tcon_ipc)
                                      // UAF
     ses->tcon_ipc = NULLl
                                   spin_unlock(&cifs_tcp_ses_lock);

   spin_lock(&cifs_tcp_ses_lock)
   list_del_init(&ses->smb_ses_list)
   spin_unlock(&cifs_tcp_ses_lock)
---

In commit ff7d80a9f271 ("cifs: fix session state transition to avoid use-after-free
issue"), setting ses_status to SES_EXITING was moved under the protection of
cifs_tcp_ses_lock.

In cifs_debug_data_proc_show(), the logic that checks ses->ses_status == SES_EXITING
already seems sufficient to avoid this issue. Therefore, it appears that ses->ses_lock
might not be necessary. Additionally, I am curious why ses->ses_lock needs to cover
such a large scope.


> diff --git a/fs/smb/client/cifs_debug.c b/fs/smb/client/cifs_debug.c
> index 76922fcc4bc6..9a0ccd87468e 100644
> --- a/fs/smb/client/cifs_debug.c
> +++ b/fs/smb/client/cifs_debug.c
> @@ -452,6 +452,11 @@ static int cifs_debug_data_proc_show(struct seq_file *m, void *v)
>   		seq_printf(m, "\n\n\tSessions: ");
>   		i = 0;
>   		list_for_each_entry(ses, &server->smb_ses_list, smb_ses_list) {
> +			spin_lock(&ses->ses_lock);
> +			if (ses->ses_status == SES_EXITING) {
> +				spin_unlock(&ses->ses_lock);
> +				continue;
> +			}
>   			i++;
>   			if ((ses->serverDomain == NULL) ||
>   				(ses->serverOS == NULL) ||
> @@ -472,6 +477,7 @@ static int cifs_debug_data_proc_show(struct seq_file *m, void *v)
>   				ses->ses_count, ses->serverOS, ses->serverNOS,
>   				ses->capabilities, ses->ses_status);
>   			}
> +			spin_unlock(&ses->ses_lock);
>   
>   			seq_printf(m, "\n\tSecurity type: %s ",
>   				get_security_type_str(server->ops->select_sectype(server, ses->sectype)));

I believe in the latest mainline, this could potentially be modified to:

```
diff --git a/fs/smb/client/cifs_debug.c b/fs/smb/client/cifs_debug.c
index c71ae5c04306..2d9e83b71643 100644
--- a/fs/smb/client/cifs_debug.c
+++ b/fs/smb/client/cifs_debug.c
@@ -485,11 +485,8 @@ static int cifs_debug_data_proc_show(struct seq_file *m, void *v)
  		seq_printf(m, "\n\n\tSessions: ");
  		i = 0;
  		list_for_each_entry(ses, &server->smb_ses_list, smb_ses_list) {
-			spin_lock(&ses->ses_lock);
-			if (ses->ses_status == SES_EXITING) {
-				spin_unlock(&ses->ses_lock);
+			if (cifs_ses_exiting(ses))
  				continue;
-			}
  			i++;
  			if ((ses->serverDomain == NULL) ||
  				(ses->serverOS == NULL) ||
@@ -512,7 +509,6 @@ static int cifs_debug_data_proc_show(struct seq_file *m, void *v)
  			}
  			if (ses->expired_pwd)
  				seq_puts(m, "password no longer valid ");
-			spin_unlock(&ses->ses_lock);
  
  			seq_printf(m, "\n\tSecurity type: %s ",
  				get_security_type_str(server->ops->select_sectype(server, ses->sectype)));

```

Best regards,
Wang Zhaolong