| Message ID | 20240402193404.236159-1-pc@manguebit.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [01/12] smb: client: fix potential UAF in cifs_debug_files_proc_show() | expand |
Hello,
I have some questions regarding CVE-2024-26928.
I would like to confirm whether the phrase "fix potential UAF in
cifs_debug_files_proc_show()" implies that the UAF issue does not
actually exist, correct?
Based on my analysis, `cifs_tcp_ses_lock` plays a crucial role in
preventing the UAF.
After adding `ses` to the `smb_ses_list` list, the only place where
the `ses` is freed is in the `__cifs_put_smb_ses` function:
```
__cifs_put_smb_ses()
...
spin_lock(&cifs_tcp_ses_lock);
list_del_init(&ses->smb_ses_list);
spin_unlock(&cifs_tcp_ses_lock);
...
sesInfoFree(ses);
```
The `ses` is freed only after it is removed from the list, and this
removal is protected by `cifs_tcp_ses_lock`.
In `cifs_debug_files_proc_show()`, the `cifs_tcp_ses_lock` is still
held, ensuring that during access to the `ses` that is about to be
destroyed, the `ses` will not be freed in `__cifs_put_smb_ses()`,
thus preventing a UAF issue.
```
cifs_debug_files_proc_show()
...
spin_lock(&cifs_tcp_ses_lock);
...
list_for_each_entry(ses, &server->smb_ses_list, smb_ses_list)
...
spin_lock(&cifs_tcp_ses_lock);
```
Based on this understanding, I wonder if the issue addressed by
this CVE might not be a genuine problem. I am also curious about
the series of patches considered as fixes for this CVE.
Best regards,
Wang Zhaolong
> Skip sessions that are being teared down (status == SES_EXITING) to
> avoid UAF.
>
> Signed-off-by: Paulo Alcantara (Red Hat) <pc@manguebit.com>
> ---
> fs/smb/client/cifs_debug.c | 2 ++
> fs/smb/client/cifsglob.h | 10 ++++++++++
> 2 files changed, 12 insertions(+)
>
> diff --git a/fs/smb/client/cifs_debug.c b/fs/smb/client/cifs_debug.c
> index 226d4835c92d..c9aec9a38ad3 100644
> --- a/fs/smb/client/cifs_debug.c
> +++ b/fs/smb/client/cifs_debug.c
> @@ -250,6 +250,8 @@ static int cifs_debug_files_proc_show(struct seq_file *m, void *v)
> spin_lock(&cifs_tcp_ses_lock);
> list_for_each_entry(server, &cifs_tcp_ses_list, tcp_ses_list) {
> list_for_each_entry(ses, &server->smb_ses_list, smb_ses_list) {
> + if (cifs_ses_exiting(ses))
> + continue;
> list_for_each_entry(tcon, &ses->tcon_list, tcon_list) {
> spin_lock(&tcon->open_file_lock);
> list_for_each_entry(cfile, &tcon->openFileList, tlist) {
> diff --git a/fs/smb/client/cifsglob.h b/fs/smb/client/cifsglob.h
> index 286afbe346be..f67607319c43 100644
> --- a/fs/smb/client/cifsglob.h
> +++ b/fs/smb/client/cifsglob.h
> @@ -2322,4 +2322,14 @@ struct smb2_compound_vars {
> struct kvec ea_iov;
> };
>
> +static inline bool cifs_ses_exiting(struct cifs_ses *ses)
> +{
> + bool ret;
> +
> + spin_lock(&ses->ses_lock);
> + ret = ses->ses_status == SES_EXITING;
> + spin_unlock(&ses->ses_lock);
> + return ret;
> +}
> +
> #endif /* _CIFS_GLOB_H */
Wang Zhaolong <wangzhaolong1@huawei.com> writes: > Hello, > > I have some questions regarding CVE-2024-26928. > > I would like to confirm whether the phrase "fix potential UAF in > cifs_debug_files_proc_show()" implies that the UAF issue does not > actually exist, correct? Correct. This is just a way to prevent one from accessing any fields from @ses while it is being released by a different task. > Based on this understanding, I wonder if the issue addressed by > this CVE might not be a genuine problem. I am also curious about > the series of patches considered as fixes for this CVE. Nope. The fixes were created and sent without having any related CVEs.
diff --git a/fs/smb/client/cifs_debug.c b/fs/smb/client/cifs_debug.c index 226d4835c92d..c9aec9a38ad3 100644 --- a/fs/smb/client/cifs_debug.c +++ b/fs/smb/client/cifs_debug.c @@ -250,6 +250,8 @@ static int cifs_debug_files_proc_show(struct seq_file *m, void *v) spin_lock(&cifs_tcp_ses_lock); list_for_each_entry(server, &cifs_tcp_ses_list, tcp_ses_list) { list_for_each_entry(ses, &server->smb_ses_list, smb_ses_list) { + if (cifs_ses_exiting(ses)) + continue; list_for_each_entry(tcon, &ses->tcon_list, tcon_list) { spin_lock(&tcon->open_file_lock); list_for_each_entry(cfile, &tcon->openFileList, tlist) { diff --git a/fs/smb/client/cifsglob.h b/fs/smb/client/cifsglob.h index 286afbe346be..f67607319c43 100644 --- a/fs/smb/client/cifsglob.h +++ b/fs/smb/client/cifsglob.h @@ -2322,4 +2322,14 @@ struct smb2_compound_vars { struct kvec ea_iov; }; +static inline bool cifs_ses_exiting(struct cifs_ses *ses) +{ + bool ret; + + spin_lock(&ses->ses_lock); + ret = ses->ses_status == SES_EXITING; + spin_unlock(&ses->ses_lock); + return ret; +} + #endif /* _CIFS_GLOB_H */
Skip sessions that are being teared down (status == SES_EXITING) to avoid UAF. Signed-off-by: Paulo Alcantara (Red Hat) <pc@manguebit.com> --- fs/smb/client/cifs_debug.c | 2 ++ fs/smb/client/cifsglob.h | 10 ++++++++++ 2 files changed, 12 insertions(+)