[PATCH 01/12] smb: client: fix potential UAF in cifs_debug_files_proc_show()

[PATCH 01/12] smb: client: fix potential UAF in cifs_debug_files_proc_show()

[Paulo Alcantara]

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 */
-- 
2.44.0

[PATCH 02/12] smb: client: fix potential UAF in cifs_dump_full_key()

[Paulo Alcantara]

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/ioctl.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/fs/smb/client/ioctl.c b/fs/smb/client/ioctl.c
index c012dfdba80d..855ac5a62edf 100644
--- a/fs/smb/client/ioctl.c
+++ b/fs/smb/client/ioctl.c
@@ -247,7 +247,9 @@ static int cifs_dump_full_key(struct cifs_tcon *tcon, struct smb3_full_key_debug
 		spin_lock(&cifs_tcp_ses_lock);
 		list_for_each_entry(server_it, &cifs_tcp_ses_list, tcp_ses_list) {
 			list_for_each_entry(ses_it, &server_it->smb_ses_list, smb_ses_list) {
-				if (ses_it->Suid == out.session_id) {
+				spin_lock(&ses_it->ses_lock);
+				if (ses_it->ses_status != SES_EXITING &&
+				    ses_it->Suid == out.session_id) {
 					ses = ses_it;
 					/*
 					 * since we are using the session outside the crit
@@ -255,9 +257,11 @@ static int cifs_dump_full_key(struct cifs_tcon *tcon, struct smb3_full_key_debug
 					 * so increment its refcount
 					 */
 					cifs_smb_ses_inc_refcount(ses);
+					spin_unlock(&ses_it->ses_lock);
 					found = true;
 					goto search_end;
 				}
+				spin_unlock(&ses_it->ses_lock);
 			}
 		}
 search_end:
-- 
2.44.0

[PATCH 03/12] smb: client: fix potential UAF in cifs_stats_proc_write()

[Paulo Alcantara]

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 ++
 1 file changed, 2 insertions(+)

diff --git a/fs/smb/client/cifs_debug.c b/fs/smb/client/cifs_debug.c
index c9aec9a38ad3..8535c9907462 100644
--- a/fs/smb/client/cifs_debug.c
+++ b/fs/smb/client/cifs_debug.c
@@ -678,6 +678,8 @@ static ssize_t cifs_stats_proc_write(struct file *file,
 			}
 #endif /* CONFIG_CIFS_STATS2 */
 			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) {
 					atomic_set(&tcon->num_smbs_sent, 0);
 					spin_lock(&tcon->stat_lock);
-- 
2.44.0

[PATCH 04/12] smb: client: fix potential UAF in cifs_stats_proc_show()

[Paulo Alcantara]

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 ++
 1 file changed, 2 insertions(+)

diff --git a/fs/smb/client/cifs_debug.c b/fs/smb/client/cifs_debug.c
index 8535c9907462..c71ae5c04306 100644
--- a/fs/smb/client/cifs_debug.c
+++ b/fs/smb/client/cifs_debug.c
@@ -759,6 +759,8 @@ static int cifs_stats_proc_show(struct seq_file *m, void *v)
 			}
 #endif /* STATS2 */
 		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) {
 				i++;
 				seq_printf(m, "\n%d) %s", i, tcon->tree_name);
-- 
2.44.0

[PATCH 05/12] smb: client: fix potential UAF in smb2_check_message()

[Paulo Alcantara]

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/smb2misc.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/fs/smb/client/smb2misc.c b/fs/smb/client/smb2misc.c
index 82b84a4941dd..14d74ef70cc8 100644
--- a/fs/smb/client/smb2misc.c
+++ b/fs/smb/client/smb2misc.c
@@ -160,7 +160,8 @@ smb2_check_message(char *buf, unsigned int len, struct TCP_Server_Info *server)
 		/* decrypt frame now that it is completely read in */
 		spin_lock(&cifs_tcp_ses_lock);
 		list_for_each_entry(iter, &pserver->smb_ses_list, smb_ses_list) {
-			if (iter->Suid == le64_to_cpu(thdr->SessionId)) {
+			if (!cifs_ses_exiting(iter) &&
+			    iter->Suid == le64_to_cpu(thdr->SessionId)) {
 				ses = iter;
 				break;
 			}
-- 
2.44.0

[PATCH 06/12] smb: client: fix potential UAF in smb2_is_valid_lease_break()

[Paulo Alcantara]

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/smb2misc.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/fs/smb/client/smb2misc.c b/fs/smb/client/smb2misc.c
index 14d74ef70cc8..4abbf6545c9c 100644
--- a/fs/smb/client/smb2misc.c
+++ b/fs/smb/client/smb2misc.c
@@ -623,6 +623,8 @@ smb2_is_valid_lease_break(char *buffer, struct TCP_Server_Info *server)
 	/* look up tcon based on tid & uid */
 	spin_lock(&cifs_tcp_ses_lock);
 	list_for_each_entry(ses, &pserver->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);
 			cifs_stats_inc(
-- 
2.44.0

[PATCH 07/12] smb: client: fix potential UAF in smb2_is_valid_oplock_break()

[Paulo Alcantara]

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/smb2misc.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/fs/smb/client/smb2misc.c b/fs/smb/client/smb2misc.c
index 4abbf6545c9c..29b5ae881d48 100644
--- a/fs/smb/client/smb2misc.c
+++ b/fs/smb/client/smb2misc.c
@@ -700,6 +700,8 @@ smb2_is_valid_oplock_break(char *buffer, struct TCP_Server_Info *server)
 	/* look up tcon based on tid & uid */
 	spin_lock(&cifs_tcp_ses_lock);
 	list_for_each_entry(ses, &pserver->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);
-- 
2.44.0

[PATCH 08/12] smb: client: fix potential UAF in is_valid_oplock_break()

[Paulo Alcantara]

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/misc.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/fs/smb/client/misc.c b/fs/smb/client/misc.c
index 1ea22b3955a2..33ac4f8f5050 100644
--- a/fs/smb/client/misc.c
+++ b/fs/smb/client/misc.c
@@ -481,6 +481,8 @@ is_valid_oplock_break(char *buffer, struct TCP_Server_Info *srv)
 	/* look up tcon based on tid & uid */
 	spin_lock(&cifs_tcp_ses_lock);
 	list_for_each_entry(ses, &pserver->smb_ses_list, smb_ses_list) {
+		if (cifs_ses_exiting(ses))
+			continue;
 		list_for_each_entry(tcon, &ses->tcon_list, tcon_list) {
 			if (tcon->tid != buf->Tid)
 				continue;
-- 
2.44.0

[PATCH 09/12] smb: client: fix potential UAF in smb2_get_sign_key()

[Paulo Alcantara]

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/smb2transport.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/fs/smb/client/smb2transport.c b/fs/smb/client/smb2transport.c
index 1d6e54f7879e..400175b9ef47 100644
--- a/fs/smb/client/smb2transport.c
+++ b/fs/smb/client/smb2transport.c
@@ -89,8 +89,10 @@ int smb2_get_sign_key(__u64 ses_id, struct TCP_Server_Info *server, u8 *key)
 	pserver = SERVER_IS_CHAN(server) ? server->primary_server : server;
 
 	list_for_each_entry(ses, &pserver->smb_ses_list, smb_ses_list) {
-		if (ses->Suid == ses_id)
+		spin_lock(&ses->ses_lock);
+		if (ses->ses_status != SES_EXITING && ses->Suid == ses_id)
 			goto found;
+		spin_unlock(&ses->ses_lock);
 	}
 	trace_smb3_ses_not_found(ses_id);
 	cifs_server_dbg(FYI, "%s: Could not find session 0x%llx\n",
@@ -99,7 +101,6 @@ int smb2_get_sign_key(__u64 ses_id, struct TCP_Server_Info *server, u8 *key)
 	goto out;
 
 found:
-	spin_lock(&ses->ses_lock);
 	spin_lock(&ses->chan_lock);
 
 	is_binding = (cifs_chan_needs_reconnect(ses, server) &&
-- 
2.44.0

Re: [PATCH 09/12] smb: client: fix potential UAF in smb2_get_sign_key()

[Paulo Alcantara]

Paulo Alcantara <pc@manguebit.com> writes:

> 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/smb2transport.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)

Please ignore this one as we still need the signing key for session
logoff in __cifs_put_smb_ses().

[PATCH 10/12] smb: client: fix potential UAF in smb2_is_network_name_deleted()

[Paulo Alcantara]

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/smb2ops.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/fs/smb/client/smb2ops.c b/fs/smb/client/smb2ops.c
index 35bf7eb315cd..1506a0eb10ba 100644
--- a/fs/smb/client/smb2ops.c
+++ b/fs/smb/client/smb2ops.c
@@ -2480,6 +2480,8 @@ smb2_is_network_name_deleted(char *buf, struct TCP_Server_Info *server)
 
 	spin_lock(&cifs_tcp_ses_lock);
 	list_for_each_entry(ses, &pserver->smb_ses_list, smb_ses_list) {
+		if (cifs_ses_exiting(ses))
+			continue;
 		list_for_each_entry(tcon, &ses->tcon_list, tcon_list) {
 			if (tcon->tid == le32_to_cpu(shdr->Id.SyncId.TreeId)) {
 				spin_lock(&tcon->tc_lock);
-- 
2.44.0

[PATCH 11/12] smb: client: fix potential UAF in smb2_get_enc_key()

[Paulo Alcantara]

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/smb2ops.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/fs/smb/client/smb2ops.c b/fs/smb/client/smb2ops.c
index 1506a0eb10ba..4fd2ffa2ebba 100644
--- a/fs/smb/client/smb2ops.c
+++ b/fs/smb/client/smb2ops.c
@@ -4188,8 +4188,8 @@ smb2_get_enc_key(struct TCP_Server_Info *server, __u64 ses_id, int enc, u8 *key)
 
 	spin_lock(&cifs_tcp_ses_lock);
 	list_for_each_entry(ses, &pserver->smb_ses_list, smb_ses_list) {
-		if (ses->Suid == ses_id) {
-			spin_lock(&ses->ses_lock);
+		spin_lock(&ses->ses_lock);
+		if (ses->ses_status != SES_EXITING && ses->Suid == ses_id) {
 			ses_enc_key = enc ? ses->smb3encryptionkey :
 				ses->smb3decryptionkey;
 			memcpy(key, ses_enc_key, SMB3_ENC_DEC_KEY_SIZE);
@@ -4197,6 +4197,7 @@ smb2_get_enc_key(struct TCP_Server_Info *server, __u64 ses_id, int enc, u8 *key)
 			spin_unlock(&cifs_tcp_ses_lock);
 			return 0;
 		}
+		spin_unlock(&ses->ses_lock);
 	}
 	spin_unlock(&cifs_tcp_ses_lock);
 
-- 
2.44.0

Re: [PATCH 11/12] smb: client: fix potential UAF in smb2_get_enc_key()

[Steve French]

Isn't this needed to send the SMB3 Logoff request?

On Tue, Apr 2, 2024 at 2:35 PM Paulo Alcantara <pc@manguebit.com> wrote:
>
> 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/smb2ops.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/fs/smb/client/smb2ops.c b/fs/smb/client/smb2ops.c
> index 1506a0eb10ba..4fd2ffa2ebba 100644
> --- a/fs/smb/client/smb2ops.c
> +++ b/fs/smb/client/smb2ops.c
> @@ -4188,8 +4188,8 @@ smb2_get_enc_key(struct TCP_Server_Info *server, __u64 ses_id, int enc, u8 *key)
>
>         spin_lock(&cifs_tcp_ses_lock);
>         list_for_each_entry(ses, &pserver->smb_ses_list, smb_ses_list) {
> -               if (ses->Suid == ses_id) {
> -                       spin_lock(&ses->ses_lock);
> +               spin_lock(&ses->ses_lock);
> +               if (ses->ses_status != SES_EXITING && ses->Suid == ses_id) {
>                         ses_enc_key = enc ? ses->smb3encryptionkey :
>                                 ses->smb3decryptionkey;
>                         memcpy(key, ses_enc_key, SMB3_ENC_DEC_KEY_SIZE);
> @@ -4197,6 +4197,7 @@ smb2_get_enc_key(struct TCP_Server_Info *server, __u64 ses_id, int enc, u8 *key)
>                         spin_unlock(&cifs_tcp_ses_lock);
>                         return 0;
>                 }
> +               spin_unlock(&ses->ses_lock);
>         }
>         spin_unlock(&cifs_tcp_ses_lock);
>
> --
> 2.44.0
>
>


-- 
Thanks,

Steve

Re: [PATCH 11/12] smb: client: fix potential UAF in smb2_get_enc_key()

[Paulo Alcantara]

Steve French <smfrench@gmail.com> writes:

> Isn't this needed to send the SMB3 Logoff request?

Yes, good catch!  Please drop this one.

Re: [PATCH 11/12] smb: client: fix potential UAF in smb2_get_enc_key()

[Paulo Alcantara]

Paulo Alcantara <pc@manguebit.com> writes:

> 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/smb2ops.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)

As we can send encrypted session logoff when
SMB2_SESSION_FLAG_ENCRYPT_DATA is set, then please ignore this one and
patch 05/12.

[PATCH 12/12] smb: client: fix potential UAF in cifs_signal_cifsd_for_reconnect()

[Paulo Alcantara]

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/connect.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/fs/smb/client/connect.c b/fs/smb/client/connect.c
index 95e4bda4fd51..85679ae106fd 100644
--- a/fs/smb/client/connect.c
+++ b/fs/smb/client/connect.c
@@ -175,6 +175,8 @@ cifs_signal_cifsd_for_reconnect(struct TCP_Server_Info *server,
 
 	spin_lock(&cifs_tcp_ses_lock);
 	list_for_each_entry(ses, &pserver->smb_ses_list, smb_ses_list) {
+		if (cifs_ses_exiting(ses))
+			continue;
 		spin_lock(&ses->chan_lock);
 		for (i = 0; i < ses->chan_count; i++) {
 			if (!ses->chans[i].server)
-- 
2.44.0

Re: [PATCH 01/12] smb: client: fix potential UAF in cifs_debug_files_proc_show()

[Wang Zhaolong]

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 */

Re: [PATCH 01/12] smb: client: fix potential UAF in cifs_debug_files_proc_show()

[Paulo Alcantara]

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.