| Message ID | 20231030201956.2660-2-pc@manguebit.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/4] smb: client: remove extra @chan_count check in __cifs_put_smb_ses() | expand |
fix was already in cifs-2.6.git for-next Added Cc: stable to it though Let me know if you see something missing (or if patch changed from the previous one eg) On Mon, Oct 30, 2023 at 3:20 PM Paulo Alcantara <pc@manguebit.com> wrote: > > Skip SMB sessions that are being teared down > (e.g. @ses->ses_status == SES_EXITING) in cifs_debug_data_proc_show() > to avoid use-after-free in @ses. > > This fixes the following GPF when reading from /proc/fs/cifs/DebugData > while mounting and umounting > > [ 816.251274] general protection fault, probably for non-canonical > address 0x6b6b6b6b6b6b6d81: 0000 [#1] PREEMPT SMP NOPTI > ... > [ 816.260138] Call Trace: > [ 816.260329] <TASK> > [ 816.260499] ? die_addr+0x36/0x90 > [ 816.260762] ? exc_general_protection+0x1b3/0x410 > [ 816.261126] ? asm_exc_general_protection+0x26/0x30 > [ 816.261502] ? cifs_debug_tcon+0xbd/0x240 [cifs] > [ 816.261878] ? cifs_debug_tcon+0xab/0x240 [cifs] > [ 816.262249] cifs_debug_data_proc_show+0x516/0xdb0 [cifs] > [ 816.262689] ? seq_read_iter+0x379/0x470 > [ 816.262995] seq_read_iter+0x118/0x470 > [ 816.263291] proc_reg_read_iter+0x53/0x90 > [ 816.263596] ? srso_alias_return_thunk+0x5/0x7f > [ 816.263945] vfs_read+0x201/0x350 > [ 816.264211] ksys_read+0x75/0x100 > [ 816.264472] do_syscall_64+0x3f/0x90 > [ 816.264750] entry_SYSCALL_64_after_hwframe+0x6e/0xd8 > [ 816.265135] RIP: 0033:0x7fd5e669d381 > > Cc: Frank Sorenson <sorenson@redhat.com> > Signed-off-by: Paulo Alcantara (SUSE) <pc@manguebit.com> > --- > fs/smb/client/cifs_debug.c | 6 ++++++ > 1 file changed, 6 insertions(+) > > 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))); > -- > 2.42.0 >
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
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)));
Skip SMB sessions that are being teared down (e.g. @ses->ses_status == SES_EXITING) in cifs_debug_data_proc_show() to avoid use-after-free in @ses. This fixes the following GPF when reading from /proc/fs/cifs/DebugData while mounting and umounting [ 816.251274] general protection fault, probably for non-canonical address 0x6b6b6b6b6b6b6d81: 0000 [#1] PREEMPT SMP NOPTI ... [ 816.260138] Call Trace: [ 816.260329] <TASK> [ 816.260499] ? die_addr+0x36/0x90 [ 816.260762] ? exc_general_protection+0x1b3/0x410 [ 816.261126] ? asm_exc_general_protection+0x26/0x30 [ 816.261502] ? cifs_debug_tcon+0xbd/0x240 [cifs] [ 816.261878] ? cifs_debug_tcon+0xab/0x240 [cifs] [ 816.262249] cifs_debug_data_proc_show+0x516/0xdb0 [cifs] [ 816.262689] ? seq_read_iter+0x379/0x470 [ 816.262995] seq_read_iter+0x118/0x470 [ 816.263291] proc_reg_read_iter+0x53/0x90 [ 816.263596] ? srso_alias_return_thunk+0x5/0x7f [ 816.263945] vfs_read+0x201/0x350 [ 816.264211] ksys_read+0x75/0x100 [ 816.264472] do_syscall_64+0x3f/0x90 [ 816.264750] entry_SYSCALL_64_after_hwframe+0x6e/0xd8 [ 816.265135] RIP: 0033:0x7fd5e669d381 Cc: Frank Sorenson <sorenson@redhat.com> Signed-off-by: Paulo Alcantara (SUSE) <pc@manguebit.com> --- fs/smb/client/cifs_debug.c | 6 ++++++ 1 file changed, 6 insertions(+)