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
