cifs: fix potential use-after-free bugs in TCP_Server_Info::hostname

jira VULN-169227
cve CVE-2023-53751
commit-author Paulo Alcantara <pc@manguebit.com>
commit 90c49fc
upstream-diff |
  fs/cifs/cifsglob.h
        Added `srv_lock' to the `TCP_Server_Info' struct as it was done in
        the non-backported commit d7d7a66
        ("cifs: avoid use of global locks for high contention data").
  fs/cifs/cifs_debug.c
        Ignored changes in `cifs_debug_data_proc_show()' because the code
        using `hostname' was only added in the non-backported commit
        40f077a ("cifs: clarify hostname
        vs ip address in /proc/fs/cifs/DebugData").
  fs/cifs/connect.c
        - Changed the `reconn_set_next_dfs_target()' function instead of
          `__reconnect_target_unlocked()' which doesn't exist in LTS
          8.6. The `__reconnect_target_unlocked()' is where the `hostname'
          is "updated once or many times during reconnect" (see original
          commit message). The "reconnect" refers to the function
          `cifs_reconnect()', which calls `__reconnect_target_unlocked()'
          on a third level down the call tree.  Relative to LTS 8.6 the
          `cifs_reconnect()' underwent major rewrites in the commits
          bbcce36 ("cifs: split out dfs
          code from cifs_reconnect()") and
          c88f7dc ("cifs: support nested
          dfs links over reconnect"). In LTS 8.6 the updating of
          `hostname' can be tracked down to the
          `reconn_set_next_dfs_target()' function called by
          `cifs_reconnect()'. Apart from guarding the `server->hostname'
          value change some elements of the commit bbcce36 were included
          as well:
          - Used helper local variable `hostname' to store the value of
            `extract_hostname()' to avoid calling it while locked, which
            could lead to a schedule while atomic.
          - Avoided assigning to `server->hostname' the result of
            `extract_hostname()' call in case it ended with
            error. Otherwise `server->hostname' would contain
            rubbish. Added a debug message informing about that case.
        - Added initialization of `TCP_Server_Info::srv_lock' in
          `cifs_get_tcp_session()' as it's done in d7d7a66.
        - In `match_server()' sandwiched the `strcasecmp(server->hostname,
          ...)' call in the lock/unlock pair of `server->srv_lock',
          because in LTS 8.6 the `match_server()' is not called with the
          `server->srv_lock' held and yet this is the lock chosen to
          protect the `server->hostname' field.
  fs/cifs/sess.c
        Ignored changes in `cifs_try_adding_channels()' because the code
        using `hostname' was only added in the non-backported commit
        9c2dc11 ("smb3: do not attempt
        multichannel to server which does not support it"). As a result no
        changes to the file have been made.

TCP_Server_Info::hostname may be updated once or many times during
reconnect, so protect its access outside reconnect path as well and
then prevent any potential use-after-free bugs.

	Cc: stable@vger.kernel.org
	Signed-off-by: Paulo Alcantara (SUSE) <pc@manguebit.com>
	Signed-off-by: Steve French <stfrench@microsoft.com>
(cherry picked from commit 90c49fc)
	Signed-off-by: Marcin Wcisło <marcin.wcislo@conclusive.pl>

Author: pvts-mat

fs/cifs/cifs_debug.c

@@ -609,10 +609,13 @@ static int cifs_stats_proc_show(struct seq_file *m, void *v)
 				server->fastest_cmd[j],
 				server->slowest_cmd[j]);
 		for (j = 0; j < NUMBER_OF_SMB2_COMMANDS; j++)
-			if (atomic_read(&server->smb2slowcmd[j]))
+			if (atomic_read(&server->smb2slowcmd[j])) {
+				spin_lock(&server->srv_lock);
 				seq_printf(m, "  %d slow responses from %s for command %d\n",
 					atomic_read(&server->smb2slowcmd[j]),
 					server->hostname, j);
+				spin_unlock(&server->srv_lock);
+			}
 #endif /* STATS2 */
 		list_for_each(tmp2, &server->smb_ses_list) {
 			ses = list_entry(tmp2, struct cifs_ses,

fs/cifs/cifs_debug.h

@@ -95,19 +95,19 @@ do {									\
 
 #define cifs_server_dbg_func(ratefunc, type, fmt, ...)			\
 do {									\
-	const char *sn = "";						\
-	if (server && server->hostname)					\
-		sn = server->hostname;					\
+	spin_lock(&server->srv_lock);					\
 	if ((type) & FYI && cifsFYI & CIFS_INFO) {			\
 		pr_debug_ ## ratefunc("%s: \\\\%s " fmt,		\
-				      __FILE__, sn, ##__VA_ARGS__);	\
+				      __FILE__, server->hostname,	\
+				      ##__VA_ARGS__);			\
 	} else if ((type) & VFS) {					\
 		pr_err_ ## ratefunc("VFS: \\\\%s " fmt,			\
-				    sn, ##__VA_ARGS__);			\
+				    server->hostname, ##__VA_ARGS__);	\
 	} else if ((type) & NOISY && (NOISY != 0)) {			\
 		pr_debug_ ## ratefunc("\\\\%s " fmt,			\
-				      sn, ##__VA_ARGS__);		\
+				      server->hostname, ##__VA_ARGS__);	\
 	}								\
+	spin_unlock(&server->srv_lock);					\
 } while (0)
 
 #define cifs_server_dbg(type, fmt, ...)					\

fs/cifs/cifsglob.h

@@ -580,6 +580,7 @@ inc_rfc1001_len(void *buf, int count)
 struct TCP_Server_Info {
 	struct list_head tcp_ses_list;
 	struct list_head smb_ses_list;
+	spinlock_t srv_lock;  /* protect anything here that is not protected */
 	int srv_count; /* reference counter */
 	/* 15 character server name + 0x20 16th byte indicating type = srv */
 	char server_RFC1001_name[RFC1001_NAME_LEN_WITH_NULL];

fs/cifs/connect.c

@@ -130,6 +130,7 @@ static void reconn_set_next_dfs_target(struct TCP_Server_Info *server,
 				       struct dfs_cache_tgt_iterator **tgt_it)
 {
 	const char *name;
+	char *hostname;
 	int rc;
 
 	if (!cifs_sb || !cifs_sb->origin_fullpath)
@@ -147,13 +148,18 @@ static void reconn_set_next_dfs_target(struct TCP_Server_Info *server,
 
 	name = dfs_cache_get_tgt_name(*tgt_it);
 
-	kfree(server->hostname);
-
-	server->hostname = extract_hostname(name);
-	if (IS_ERR(server->hostname)) {
+	hostname = extract_hostname(name);
+	if (!IS_ERR(hostname)) {
+		spin_lock(&server->srv_lock);
+		kfree(server->hostname);
+		server->hostname = hostname;
+		spin_unlock(&server->srv_lock);
+	} else {
 		cifs_dbg(FYI,
 			 "%s: failed to extract hostname from target: %ld\n",
-			 __func__, PTR_ERR(server->hostname));
+			 __func__, PTR_ERR(hostname));
+		cifs_dbg(FYI, "%s: default to last target server: %s\n", __func__,
+			 server->hostname);
 	}
 
 	rc = reconn_set_ipaddr_from_hostname(server);
@@ -418,9 +424,7 @@ cifs_echo_request(struct work_struct *work)
 		goto requeue_echo;
 
 	rc = server->ops->echo ? server->ops->echo(server) : -ENOSYS;
-	if (rc)
-		cifs_dbg(FYI, "Unable to send echo request to server: %s\n",
-			 server->hostname);
+	cifs_server_dbg(FYI, "send echo request: rc = %d\n", rc);
 
 #ifdef CONFIG_CIFS_SWN_UPCALL
 	/* Check witness registrations */
@@ -1194,8 +1198,12 @@ static int match_server(struct TCP_Server_Info *server, struct smb3_fs_context *
 	if (!net_eq(cifs_net_ns(server), current->nsproxy->net_ns))
 		return 0;
 
-	if (strcasecmp(server->hostname, ctx->server_hostname))
+	spin_lock(&server->srv_lock);
+	if (strcasecmp(server->hostname, ctx->server_hostname)) {
+		spin_unlock(&server->srv_lock);
 		return 0;
+	}
+	spin_unlock(&server->srv_lock);
 
 	if (!match_address(server, addr,
 			   (struct sockaddr *)&ctx->srcaddr))
@@ -1343,6 +1351,7 @@ cifs_get_tcp_session(struct smb3_fs_context *ctx)
 	tcp_ses->lstrp = jiffies;
 	tcp_ses->compress_algorithm = cpu_to_le16(ctx->compression);
 	spin_lock_init(&tcp_ses->req_lock);
+	spin_lock_init(&tcp_ses->srv_lock);
 	INIT_LIST_HEAD(&tcp_ses->tcp_ses_list);
 	INIT_LIST_HEAD(&tcp_ses->smb_ses_list);
 	INIT_DELAYED_WORK(&tcp_ses->echo, cifs_echo_request);
@@ -1512,7 +1521,9 @@ cifs_setup_ipc(struct cifs_ses *ses, struct smb3_fs_context *ctx)
 	if (tcon == NULL)
 		return -ENOMEM;
 
+	spin_lock(&server->srv_lock);
 	scnprintf(unc, sizeof(unc), "\\\\%s\\IPC$", server->hostname);
+	spin_unlock(&server->srv_lock);
 
 	xid = get_xid();
 	tcon->ses = ses;