Open
Conversation
jira VULN-169227 cve-pre CVE-2023-53751 commit-author Vincent Whitchurch <vincent.whitchurch@axis.com> commit cc391b6 upstream-diff | Multiple differences across many files due to large discrepancies between upstream and LTS 8.6, HOWEVER, the commit was obtained in the exact same way as on upstream, namely: 1. locating all usages of `TCP_Server_Info::srv_mutex', 2. changing all `mutex_lock(&<server_ptr>->srv_mutex)' to `cifs_server_lock(<server_ptr>)', 3. changing all `mutex_unlock(&<server_ptr>->srv_mutex)' to `cifs_server_unlock(<server_ptr>)', 4. renaming `srv_mutex~' to `_srv_mutex' to make sure nothing was missed. In this regard the commit doesn't differ from the upstream. The srv_mutex is used during writeback so cifs should ensure that allocations done when that mutex is held are done with GFP_NOFS, to avoid having direct reclaim ending up waiting for the same mutex and causing a deadlock. This is detected by lockdep with the splat below: ====================================================== WARNING: possible circular locking dependency detected 5.18.0 ctrliq#70 Not tainted ------------------------------------------------------ kswapd0/49 is trying to acquire lock: ffff8880195782e0 (&tcp_ses->srv_mutex){+.+.}-{3:3}, at: compound_send_recv but task is already holding lock: ffffffffa98e66c0 (fs_reclaim){+.+.}-{0:0}, at: balance_pgdat which lock already depends on the new lock. the existing dependency chain (in reverse order) is: -> ctrliq#1 (fs_reclaim){+.+.}-{0:0}: fs_reclaim_acquire kmem_cache_alloc_trace __request_module crypto_alg_mod_lookup crypto_alloc_tfm_node crypto_alloc_shash cifs_alloc_hash smb311_crypto_shash_allocate smb311_update_preauth_hash compound_send_recv cifs_send_recv SMB2_negotiate smb2_negotiate cifs_negotiate_protocol cifs_get_smb_ses cifs_mount cifs_smb3_do_mount smb3_get_tree vfs_get_tree path_mount __x64_sys_mount do_syscall_64 entry_SYSCALL_64_after_hwframe -> #0 (&tcp_ses->srv_mutex){+.+.}-{3:3}: __lock_acquire lock_acquire __mutex_lock mutex_lock_nested compound_send_recv cifs_send_recv SMB2_write smb2_sync_write cifs_write cifs_writepage_locked cifs_writepage shrink_page_list shrink_lruvec shrink_node balance_pgdat kswapd kthread ret_from_fork other info that might help us debug this: Possible unsafe locking scenario: CPU0 CPU1 ---- ---- lock(fs_reclaim); lock(&tcp_ses->srv_mutex); lock(fs_reclaim); lock(&tcp_ses->srv_mutex); *** DEADLOCK *** 1 lock held by kswapd0/49: #0: ffffffffa98e66c0 (fs_reclaim){+.+.}-{0:0}, at: balance_pgdat stack backtrace: CPU: 2 PID: 49 Comm: kswapd0 Not tainted 5.18.0 ctrliq#70 Call Trace: <TASK> dump_stack_lvl dump_stack print_circular_bug.cold check_noncircular __lock_acquire lock_acquire __mutex_lock mutex_lock_nested compound_send_recv cifs_send_recv SMB2_write smb2_sync_write cifs_write cifs_writepage_locked cifs_writepage shrink_page_list shrink_lruvec shrink_node balance_pgdat kswapd kthread ret_from_fork </TASK> Fix this by using the memalloc_nofs_save/restore APIs around the places where the srv_mutex is held. Do this in a wrapper function for the lock/unlock of the srv_mutex, and rename the srv_mutex to avoid missing call sites in the conversion. Note that there is another lockdep warning involving internal crypto locks, which was masked by this problem and is visible after this fix, see the discussion in this thread: https://lore.kernel.org/all/20220523123755.GA13668@axis.com/ Link: https://lore.kernel.org/r/CANT5p=rqcYfYMVHirqvdnnca4Mo+JQSw5Qu12v=kPfpk5yhhmg@mail.gmail.com/ Reported-by: Shyam Prasad N <nspmangalore@gmail.com> Suggested-by: Lars Persson <larper@axis.com> Reviewed-by: Ronnie Sahlberg <lsahlber@redhat.com> Reviewed-by: Enzo Matsumiya <ematsumiya@suse.de> Signed-off-by: Vincent Whitchurch <vincent.whitchurch@axis.com> Signed-off-by: Steve French <stfrench@microsoft.com> (cherry picked from commit cc391b6) Signed-off-by: Marcin Wcisło <marcin.wcislo@conclusive.pl>
jira VULN-169227 cve CVE-2023-53751 commit-author Paulo Alcantara <pc@cjr.nz> commit 775e44d upstream-diff Ignored the introduction of `pserver' variable, as well as the usage of `hostname' local, as they were only needed in the upstream because of the dual source of the server hostname, introduced in the non-backported commit 9de7499 ("smb3: use netname when available on secondary channels") Serialise access of TCP_Server_Info::hostname in assemble_neg_contexts() by holding the server's mutex otherwise it might end up accessing an already-freed hostname pointer from cifs_reconnect() or cifs_resolve_server(). Signed-off-by: Paulo Alcantara (SUSE) <pc@cjr.nz> Reviewed-by: Enzo Matsumiya <ematsumiya@suse.de> Signed-off-by: Steve French <stfrench@microsoft.com> (cherry picked from commit 775e44d) Signed-off-by: Marcin Wcisło <marcin.wcislo@conclusive.pl>
jira VULN-169227 cve CVE-2023-53751 commit-author Paulo Alcantara <pc@cjr.nz> commit 39a154f upstream-diff Manually obtained commit. Upstream fixes appear in multiple places, but they all cover only the `cifs_tree_connect()' function (the CONFIG_CIFS_DFS_UPCALL=y variant) and its call tree. Same goes for the backported version, taking into account the lack of a major function rewrite in c88f7dc ("cifs: support nested dfs links over reconnect"). Upstream call tree at the moment of the fix and the changes at each level: cifs_tree_connect() Put `cifs_server_{lock/unlock}(server)' around `scnprintf(..., server->hostname);' call. tree_connect_dfs_target() No changes. __tree_connect_dfs_target() Removed `extract_unc_hostname(server->hostname, ...)' call, used full `server->hostname' instead of its substring `tcp_host'. target_share_matches_server() Put `cifs_server_{lock/unlock}(server)' around `server->hostname' accesses in conditional expression and debug message composition before the `match_target_ip()' call. match_target_ip() Put `spin_{lock/unlock}(&server->srv_lock)' around `cifs_match_ipaddr(...server->dstaddr, ....)' call. Backported version call tree: cifs_tree_connect() - Put `cifs_server_{lock/unlock}(server)' around `scnprintf(..., server->hostname)' call - like in the upstream. - Put `cifs_server_{lock/unlock}(server)' around `server->hostname' (and its substring `tcp_host') access in conditional expression and debug message composition before the `match_target_ip()' call - like in the upstream. This required moving the `extract_unc_hostname()' call inside the loop. Unlike in the upstream it was not removed entirely, opting for functional equivalence between the patched and non-patched version instead of strictly preserving backported commit's changes; perhaps at the time of the upstream fix the equality `server->hostname' = `tcp_host' held true, allowing for the reduction of `extract_unc_hostname()' call, but that was not certain for the ciqlts8_6 version. If it wasn't true then the commit mixed a bugfix with a functional change which was simply filtered out here. Moving `extract_unc_hostname()' call inside the loop did not create any algorithmic inconsistencies which weren't already present (if any), merely preventing them from ending in bad memory access. In the upstream the value of `server->hostname' isn't saved to remain constant for all loop iterations in the `__tree_connect_dfs_target()' function either. A ngeligible performance penalty associated with the calculation redundancy was accepted given the simplicity of the fix it allowed. match_target_ip() Put `spin_{lock/unlock}(&cifs_tcp_ses_lock)' around the `cifs_match_ipaddr(...server->dstaddr, ...)' call - like in the upstream, except for the `cifs_tcp_ses_lock' instead of `server->srv_lock'. The latter was introduced in the non-backported commit d7d7a66 ("cifs: avoid use of global locks for high contention data"). The same pattern of protecting `server->dstaddr' with `cifs_tcp_ses_lock' can be observed in ciqlts8_6 in the `reconn_set_ipaddr_from_hostname()' function. Use the appropriate locks to protect access of hostname and dstaddr fields in cifs_tree_connect() as they might get changed by other tasks. Signed-off-by: Paulo Alcantara (SUSE) <pc@cjr.nz> Reviewed-by: Enzo Matsumiya <ematsumiya@suse.de> Signed-off-by: Steve French <stfrench@microsoft.com> (cherry picked from commit 39a154f) Signed-off-by: Marcin Wcisło <marcin.wcislo@conclusive.pl>
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()'. - Added initialization of `TCP_Server_Info::srv_lock' in `cifs_get_tcp_session()' as it's done in d7d7a66. - In `match_server()' changed the lockdep assertion from holding `server->srv_lock' to holding `cifs_tcp_ses_lock', because this is the actual invariant for the LTS 8.6 version. - 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>
pvts-mat
force-pushed
the
ciqlts8_6-CVE-2023-53751
branch
from
377e3a8 to
ac27ef5
Compare
|
🤖 Validation Checks In Progress Workflow run: https://github.com/ctrliq/kernel-src-tree/actions/runs/23870172083 |
🔍 Upstream Linux Kernel Commit Check
This is an automated message from the kernel commit checker workflow. |
🔍 Interdiff Analysis
================================================================================
* DELTA DIFFERENCES - code changes that differ between the patches *
================================================================================
--- b/fs/cifs/sess.c
+++ b/fs/cifs/sess.c
@@ -903,14 +903,14 @@
{
struct cifs_ses *ses = sess_data->ses;
- cifs_server_lock(ses->server);
+ mutex_lock(&ses->server->srv_mutex);
if (!ses->server->session_estab) {
if (ses->server->sign) {
ses->server->session_key.response =
kmemdup(ses->auth_key.response,
ses->auth_key.len, GFP_KERNEL);
if (!ses->server->session_key.response) {
- cifs_server_unlock(ses->server);
+ mutex_unlock(&ses->server->srv_mutex);
return -ENOMEM;
}
ses->server->session_key.len =
@@ -919,7 +919,7 @@
ses->server->sequence_number = 0x2;
ses->server->session_estab = true;
}
- cifs_server_unlock(ses->server);
+ mutex_unlock(&ses->server->srv_mutex);
cifs_dbg(FYI, "CIFS session established successfully\n");
spin_lock(&GlobalMid_Lock);
################################################################################
! REJECTED PATCH2 HUNKS - could not be compared; manual review needed !
################################################################################
--- b/fs/cifs/cifs_swn.c
+++ b/fs/cifs/cifs_swn.c
@@ -465,7 +465,7 @@
int ret = 0;
/* Store the reconnect address */
- mutex_lock(&tcon->ses->server->srv_mutex);
+ cifs_server_lock(tcon->ses->server);
if (cifs_sockaddr_equal(&tcon->ses->server->dstaddr, addr))
goto unlock;
@@ -501,7 +501,7 @@
cifs_signal_cifsd_for_reconnect(tcon->ses->server, false);
unlock:
- mutex_unlock(&tcon->ses->server->srv_mutex);
+ cifs_server_unlock(tcon->ses->server);
return ret;
}
--- b/fs/cifs/cifsglob.h
+++ b/fs/cifs/cifsglob.h
@@ -16,6 +16,7 @@
#include <linux/mempool.h>
#include <linux/workqueue.h>
#include <linux/utsname.h>
+#include <linux/sched/mm.h>
#include <linux/netfs.h>
#include "cifs_fs_sb.h"
#include "cifsacl.h"
--- b/fs/cifs/connect.c
+++ b/fs/cifs/connect.c
@@ -148,7 +148,7 @@
struct TCP_Server_Info *server = container_of(work,
struct TCP_Server_Info, resolve.work);
- mutex_lock(&server->srv_mutex);
+ cifs_server_lock(server);
/*
* Resolve the hostname again to make sure that IP address is up-to-date.
@@ -159,7 +159,7 @@
__func__, rc);
}
- mutex_unlock(&server->srv_mutex);
+ cifs_server_unlock(server);
}
/*
@@ -359,7 +359,7 @@
do {
try_to_freeze();
- mutex_lock(&server->srv_mutex);
+ cifs_server_lock(server);
if (!cifs_swn_set_server_dstaddr(server)) {
/* resolve the hostname again to make sure that IP address is up-to-date */
@@ -372,7 +372,7 @@
else
rc = generic_ip_connect(server);
if (rc) {
- mutex_unlock(&server->srv_mutex);
+ cifs_server_unlock(server);
cifs_dbg(FYI, "%s: reconnect error %d\n", __func__, rc);
msleep(3000);
} else {
@@ -383,7 +383,7 @@
server->tcpStatus = CifsNeedNegotiate;
spin_unlock(&cifs_tcp_ses_lock);
cifs_swn_reset_server_dstaddr(server);
- mutex_unlock(&server->srv_mutex);
+ cifs_server_unlock(server);
mod_delayed_work(cifsiod_wq, &server->reconnect, 0);
}
} while (server->tcpStatus == CifsNeedReconnect);
@@ -488,7 +488,7 @@
do {
try_to_freeze();
- mutex_lock(&server->srv_mutex);
+ cifs_server_lock(server);
rc = reconnect_target_unlocked(server, &tl, &target_hint);
if (rc) {
@@ -492,7 +492,7 @@
rc = reconnect_target_unlocked(server, &tl, &target_hint);
if (rc) {
/* Failed to reconnect socket */
- mutex_unlock(&server->srv_mutex);
+ cifs_server_unlock(server);
cifs_dbg(FYI, "%s: reconnect error %d\n", __func__, rc);
msleep(3000);
continue;
@@ -510,7 +510,7 @@
server->tcpStatus = CifsNeedNegotiate;
spin_unlock(&cifs_tcp_ses_lock);
cifs_swn_reset_server_dstaddr(server);
- mutex_unlock(&server->srv_mutex);
+ cifs_server_unlock(server);
mod_delayed_work(cifsiod_wq, &server->reconnect, 0);
} while (server->tcpStatus == CifsNeedReconnect);
--- b/fs/cifs/sess.c
+++ b/fs/cifs/sess.c
@@ -1120,7 +1120,7 @@
struct cifs_ses *ses = sess_data->ses;
struct TCP_Server_Info *server = sess_data->server;
- mutex_lock(&server->srv_mutex);
+ cifs_server_lock(server);
if (!server->session_estab) {
if (server->sign) {
server->session_key.response =
@@ -1124,7 +1124,7 @@
kmemdup(ses->auth_key.response,
ses->auth_key.len, GFP_KERNEL);
if (!server->session_key.response) {
- mutex_unlock(&server->srv_mutex);
+ cifs_server_unlock(server);
return -ENOMEM;
}
server->session_key.len =
@@ -1136,7 +1136,7 @@
server->sequence_number = 0x2;
server->session_estab = true;
}
- mutex_unlock(&server->srv_mutex);
+ cifs_server_unlock(server);
cifs_dbg(FYI, "CIFS session established successfully\n");
return 0;
--- b/fs/cifs/transport.c
+++ b/fs/cifs/transport.c
@@ -1190,7 +1190,7 @@
if ((ses->ses_status == SES_NEW) || (optype & CIFS_NEG_OP) || (optype & CIFS_SESS_OP)) {
spin_unlock(&cifs_tcp_ses_lock);
- mutex_lock(&server->srv_mutex);
+ cifs_server_lock(server);
smb311_update_preauth_hash(ses, server, rqst[0].rq_iov, rqst[0].rq_nvec);
mutex_unlock(&server->srv_mutex);
@@ -1192,7 +1192,7 @@
mutex_lock(&server->srv_mutex);
smb311_update_preauth_hash(ses, server, rqst[0].rq_iov, rqst[0].rq_nvec);
- mutex_unlock(&server->srv_mutex);
+ cifs_server_unlock(server);
spin_lock(&cifs_tcp_ses_lock);
}
@@ -1266,7 +1266,7 @@
.iov_len = resp_iov[0].iov_len
};
spin_unlock(&cifs_tcp_ses_lock);
- mutex_lock(&server->srv_mutex);
+ cifs_server_lock(server);
smb311_update_preauth_hash(ses, server, &iov, 1);
mutex_unlock(&server->srv_mutex);
spin_lock(&cifs_tcp_ses_lock);
@@ -1268,7 +1268,7 @@
spin_unlock(&cifs_tcp_ses_lock);
mutex_lock(&server->srv_mutex);
smb311_update_preauth_hash(ses, server, &iov, 1);
- mutex_unlock(&server->srv_mutex);
+ cifs_server_unlock(server);
spin_lock(&cifs_tcp_ses_lock);
}
spin_unlock(&cifs_tcp_ses_lock);
================================================================================
* CONTEXT DIFFERENCES - surrounding code differences between the patches *
================================================================================
--- b/fs/cifs/cifs_swn.c
+++ b/fs/cifs/cifs_swn.c
@@ -462,7 +462,7 @@
-static int cifs_swn_reconnect(struct cifs_tcon *tcon, struct sockaddr_storage *addr)
-{
+ int ret = 0;
+
/* Store the reconnect address */
mutex_lock(&tcon->ses->server->srv_mutex);
- if (!cifs_sockaddr_equal(&tcon->ses->server->dstaddr, addr)) {
- int ret;
+ if (cifs_sockaddr_equal(&tcon->ses->server->dstaddr, addr))
+ goto unlock;
@@ -515,7 +498,7 @@
- tcon->ses->server->tcpStatus = CifsNeedReconnect;
- spin_unlock(&GlobalMid_Lock);
- }
+ cifs_signal_cifsd_for_reconnect(tcon->ses->server, false);
+
+unlock:
mutex_unlock(&tcon->ses->server->srv_mutex);
- return 0;
+ return ret;
}
--- b/fs/cifs/connect.c
+++ b/fs/cifs/connect.c
@@ -288,6 +293,6 @@
- mid_entry->mid_flags |= MID_DELETED;
+ mid->mid_flags |= MID_DELETED;
}
spin_unlock(&GlobalMid_Lock);
mutex_unlock(&server->srv_mutex);
cifs_dbg(FYI, "%s: issuing mid callbacks\n", __func__);
@@ -309,8 +315,7 @@
do {
try_to_freeze();
-
mutex_lock(&server->srv_mutex);
-#ifdef CONFIG_CIFS_SWN_UPCALL
- if (server->use_swn_dstaddr) {
+ if (!cifs_swn_set_server_dstaddr(server)) {
+ /* resolve the hostname again to make sure that IP address is up-to-date */
@@ -349,6 +424,6 @@
if (rc) {
- cifs_dbg(FYI, "reconnect error %d\n", rc);
+ /* Failed to reconnect socket */
mutex_unlock(&server->srv_mutex);
+ cifs_dbg(FYI, "%s: reconnect error %d\n", __func__, rc);
msleep(3000);
- } else {
- atomic_inc(&tcpSesReconnectCount);
+ continue;
@@ -355,7 +430,7 @@
-#ifdef CONFIG_CIFS_SWN_UPCALL
- server->use_swn_dstaddr = false;
-#endif
- mutex_unlock(&server->srv_mutex);
- }
+ server->tcpStatus = CifsNeedNegotiate;
+ spin_unlock(&cifs_tcp_ses_lock);
+ cifs_swn_reset_server_dstaddr(server);
+ mutex_unlock(&server->srv_mutex);
+ mod_delayed_work(cifsiod_wq, &server->reconnect, 0);
} while (server->tcpStatus == CifsNeedReconnect);
--- b/fs/cifs/sess.c
+++ b/fs/cifs/sess.c
@@ -900,12 +900,13 @@
struct cifs_ses *ses = sess_data->ses;
+ struct TCP_Server_Info *server = sess_data->server;
- mutex_lock(&ses->server->srv_mutex);
- if (!ses->server->session_estab) {
- if (ses->server->sign) {
- ses->server->session_key.response =
+ mutex_lock(&server->srv_mutex);
+ if (!server->session_estab) {
+ if (server->sign) {
+ server->session_key.response =
kmemdup(ses->auth_key.response,
ses->auth_key.len, GFP_KERNEL);
- if (!ses->server->session_key.response) {
- mutex_unlock(&ses->server->srv_mutex);
+ if (!server->session_key.response) {
+ mutex_unlock(&server->srv_mutex);
return -ENOMEM;
}
@@ -913,7 +913,8 @@
- ses->server->sequence_number = 0x2;
- ses->server->session_estab = true;
+ server->session_key.len =
+ server->sequence_number = 0x2;
+ server->session_estab = true;
}
- mutex_unlock(&ses->server->srv_mutex);
+ mutex_unlock(&server->srv_mutex);
cifs_dbg(FYI, "CIFS session established successfully\n");
- spin_lock(&GlobalMid_Lock);
+ return 0;
--- b/fs/cifs/smb2pdu.c
+++ b/fs/cifs/smb2pdu.c
@@ -1261,9 +1261,9 @@
struct cifs_ses *ses = sess_data->ses;
- struct TCP_Server_Info *server = cifs_ses_server(ses);
+ struct TCP_Server_Info *server = sess_data->server;
mutex_lock(&server->srv_mutex);
if (server->ops->generate_signingkey) {
- rc = server->ops->generate_signingkey(ses);
+ rc = server->ops->generate_signingkey(ses, server);
if (rc) {
cifs_dbg(FYI,
"SMB3 session key generation failed\n");
@@ -1281,4 +1386,4 @@
mutex_unlock(&server->srv_mutex);
cifs_dbg(FYI, "SMB2/3 session established successfully\n");
- /* keep existing ses state if binding */
+ return rc;
================================================================================
* ONLY IN PATCH2 - files not modified by patch1 *
================================================================================
--- a/fs/cifs/dfs_cache.c
+++ b/fs/cifs/dfs_cache.c
@@ -1327,9 +1327,9 @@ static bool target_share_equal(struct TCP_Server_Info *server, const char *s1, c
cifs_dbg(VFS, "%s: failed to convert address \'%s\'. skip address matching.\n",
__func__, ip);
} else {
- mutex_lock(&server->srv_mutex);
+ cifs_server_lock(server);
match = cifs_match_ipaddr((struct sockaddr *)&server->dstaddr, &sa);
- mutex_unlock(&server->srv_mutex);
+ cifs_server_unlock(server);
}
kfree(ip);
================================================================================
* DELTA DIFFERENCES - code changes that differ between the patches *
================================================================================
--- b/fs/cifs/smb2pdu.c
+++ b/fs/cifs/smb2pdu.c
@@ -549,10 +549,8 @@
} else
req->NegotiateContextCount = cpu_to_le16(4);
- cifs_server_lock(server);
ctxt_len = build_netname_ctxt((struct smb2_netname_neg_context *)pneg_ctxt,
server->hostname);
- cifs_server_unlock(server);
*total_len += ctxt_len;
pneg_ctxt += ctxt_len;
################################################################################
! REJECTED PATCH2 HUNKS - could not be compared; manual review needed !
################################################################################
--- b/fs/cifs/smb2pdu.c
+++ b/fs/cifs/smb2pdu.c
@@ -541,8 +541,6 @@
assemble_neg_contexts(struct smb2_negotiate_req *req,
struct TCP_Server_Info *server, unsigned int *total_len)
{
- char *pneg_ctxt;
- char *hostname = NULL;
unsigned int ctxt_len, neg_context_count;
if (*total_len > 200) {
@@ -544,6 +542,9 @@
char *pneg_ctxt;
char *hostname = NULL;
unsigned int ctxt_len, neg_context_count;
+ struct TCP_Server_Info *pserver;
+ char *pneg_ctxt;
+ char *hostname;
if (*total_len > 200) {
/* In case length corrupted don't want to overrun smb buffer */
@@ -574,8 +575,9 @@
* secondary channels don't have the hostname field populated
* use the hostname field in the primary channel instead
*/
- hostname = CIFS_SERVER_IS_CHAN(server) ?
- server->primary_server->hostname : server->hostname;
+ pserver = CIFS_SERVER_IS_CHAN(server) ? server->primary_server : server;
+ cifs_server_lock(pserver);
+ hostname = pserver->hostname;
if (hostname && (hostname[0] != 0)) {
ctxt_len = build_netname_ctxt((struct smb2_netname_neg_context *)pneg_ctxt,
hostname);
@@ -584,6 +586,7 @@
neg_context_count = 3;
} else
neg_context_count = 2;
+ cifs_server_unlock(pserver);
build_posix_ctxt((struct smb2_posix_neg_context *)pneg_ctxt);
*total_len += sizeof(struct smb2_posix_neg_context);
================================================================================
* CONTEXT DIFFERENCES - surrounding code differences between the patches *
================================================================================
--- b/fs/cifs/smb2pdu.c
+++ b/fs/cifs/smb2pdu.c
@@ -546,8 +572,3 @@
} else
- req->NegotiateContextCount = cpu_to_le16(4);
-
- ctxt_len = build_netname_ctxt((struct smb2_netname_neg_context *)pneg_ctxt,
- server->hostname);
- *total_len += ctxt_len;
- pneg_ctxt += ctxt_len;
+ neg_context_count = 2;
================================================================================
* DELTA DIFFERENCES - code changes that differ between the patches *
================================================================================
--- b/fs/cifs/misc.c
+++ b/fs/cifs/misc.c
@@ -1123,10 +1123,8 @@
goto out;
}
- spin_lock(&cifs_tcp_ses_lock);
*result = cifs_match_ipaddr((struct sockaddr *)&server->dstaddr,
&tipaddr);
- spin_unlock(&cifs_tcp_ses_lock);
cifs_dbg(FYI, "%s: ip addresses match: %u\n", __func__, *result);
rc = 0;
################################################################################
! REJECTED PATCH2 HUNKS - could not be compared; manual review needed !
################################################################################
--- b/fs/cifs/misc.c
+++ b/fs/cifs/misc.c
@@ -1277,6 +1277,7 @@
if (rc < 0)
return rc;
+ spin_lock(&server->srv_lock);
*result = cifs_match_ipaddr((struct sockaddr *)&server->dstaddr, (struct sockaddr *)&ss);
cifs_dbg(FYI, "%s: ip addresses match: %u\n", __func__, *result);
return 0;
@@ -1278,6 +1279,7 @@
return rc;
*result = cifs_match_ipaddr((struct sockaddr *)&server->dstaddr, (struct sockaddr *)&ss);
+ spin_unlock(&server->srv_lock);
cifs_dbg(FYI, "%s: ip addresses match: %u\n", __func__, *result);
return 0;
}
================================================================================
* CONTEXT DIFFERENCES - surrounding code differences between the patches *
================================================================================
--- b/fs/cifs/misc.c
+++ b/fs/cifs/misc.c
@@ -1120,6 +1122,5 @@
- *result = cifs_match_ipaddr((struct sockaddr *)&server->dstaddr,
- &tipaddr);
+ *result = cifs_match_ipaddr((struct sockaddr *)&server->dstaddr, (struct sockaddr *)&ss);
cifs_dbg(FYI, "%s: ip addresses match: %u\n", __func__, *result);
- rc = 0;
-
+ return 0;
+}
================================================================================
* ONLY IN PATCH1 - files not modified by patch2 *
================================================================================
--- b/fs/cifs/connect.c
+++ a/fs/cifs/connect.c
@@ -4081,9 +4081,7 @@
if (!tcon->dfs_path) {
if (tcon->ipc) {
- cifs_server_lock(server);
scnprintf(tree, MAX_TREE_SIZE, "\\\\%s\\IPC$", server->hostname);
- cifs_server_unlock(server);
rc = ops->tree_connect(xid, tcon->ses, tree, tcon, nlsc);
} else {
rc = ops->tree_connect(xid, tcon->ses, tcon->treeName, tcon, nlsc);
@@ -4097,6 +4095,8 @@
isroot = ref.server_type == DFS_TYPE_ROOT;
free_dfs_info_param(&ref);
+ extract_unc_hostname(server->hostname, &tcp_host, &tcp_host_len);
+
for (it = dfs_cache_get_tgt_iterator(&tl); it; it = dfs_cache_get_next_tgt(&tl, it)) {
bool target_match;
@@ -4114,13 +4114,10 @@
extract_unc_hostname(share, &dfs_host, &dfs_host_len);
- cifs_server_lock(server);
- extract_unc_hostname(server->hostname, &tcp_host, &tcp_host_len);
if (dfs_host_len != tcp_host_len
|| strncasecmp(dfs_host, tcp_host, dfs_host_len) != 0) {
cifs_dbg(FYI, "%s: %.*s doesn't match %.*s\n", __func__, (int)dfs_host_len,
dfs_host, (int)tcp_host_len, tcp_host);
- cifs_server_unlock(server);
rc = match_target_ip(server, dfs_host, dfs_host_len, &target_match);
if (rc) {
@@ -4132,8 +4129,7 @@
cifs_dbg(FYI, "%s: skipping target\n", __func__);
continue;
}
+ }
- } else
- cifs_server_unlock(server);
if (tcon->ipc) {
scnprintf(tree, MAX_TREE_SIZE, "\\\\%s\\IPC$", share);
================================================================================
* ONLY IN PATCH2 - files not modified by patch1 *
================================================================================
--- a/fs/cifs/dfs.c
+++ b/fs/cifs/dfs.c
@@ -327,8 +327,8 @@ static int update_server_fullpath(struct TCP_Server_Info *server, struct cifs_sb
return rc;
}
-static int target_share_matches_server(struct TCP_Server_Info *server, const char *tcp_host,
- size_t tcp_host_len, char *share, bool *target_match)
+static int target_share_matches_server(struct TCP_Server_Info *server, char *share,
+ bool *target_match)
{
int rc = 0;
const char *dfs_host;
@@ -338,13 +338,16 @@ static int target_share_matches_server(struct TCP_Server_Info *server, const cha
extract_unc_hostname(share, &dfs_host, &dfs_host_len);
/* Check if hostnames or addresses match */
- if (dfs_host_len != tcp_host_len || strncasecmp(dfs_host, tcp_host, dfs_host_len) != 0) {
- cifs_dbg(FYI, "%s: %.*s doesn't match %.*s\n", __func__, (int)dfs_host_len,
- dfs_host, (int)tcp_host_len, tcp_host);
+ cifs_server_lock(server);
+ if (dfs_host_len != strlen(server->hostname) ||
+ strncasecmp(dfs_host, server->hostname, dfs_host_len)) {
+ cifs_dbg(FYI, "%s: %.*s doesn't match %s\n", __func__,
+ (int)dfs_host_len, dfs_host, server->hostname);
rc = match_target_ip(server, dfs_host, dfs_host_len, target_match);
if (rc)
cifs_dbg(VFS, "%s: failed to match target ip: %d\n", __func__, rc);
}
+ cifs_server_unlock(server);
return rc;
}
@@ -358,13 +361,9 @@ static int __tree_connect_dfs_target(const unsigned int xid, struct cifs_tcon *t
struct cifs_ses *root_ses = CIFS_DFS_ROOT_SES(tcon->ses);
struct cifs_tcon *ipc = root_ses->tcon_ipc;
char *share = NULL, *prefix = NULL;
- const char *tcp_host;
- size_t tcp_host_len;
struct dfs_cache_tgt_iterator *tit;
bool target_match;
- extract_unc_hostname(server->hostname, &tcp_host, &tcp_host_len);
-
tit = dfs_cache_get_tgt_iterator(tl);
if (!tit) {
rc = -ENOENT;
@@ -387,8 +386,7 @@ static int __tree_connect_dfs_target(const unsigned int xid, struct cifs_tcon *t
break;
}
- rc = target_share_matches_server(server, tcp_host, tcp_host_len, share,
- &target_match);
+ rc = target_share_matches_server(server, share, &target_match);
if (rc)
break;
if (!target_match) {
@@ -497,7 +495,9 @@ int cifs_tree_connect(const unsigned int xid, struct cifs_tcon *tcon, const stru
}
if (tcon->ipc) {
+ cifs_server_lock(server);
scnprintf(tree, MAX_TREE_SIZE, "\\\\%s\\IPC$", server->hostname);
+ cifs_server_unlock(server);
rc = ops->tree_connect(xid, tcon->ses, tree, tcon, nlsc);
goto out;
}
================================================================================
* DELTA DIFFERENCES - code changes that differ between the patches *
================================================================================
--- b/fs/cifs/connect.c
+++ b/fs/cifs/connect.c
@@ -147,11 +147,9 @@
name = dfs_cache_get_tgt_name(*tgt_it);
- spin_lock(&server->srv_lock);
kfree(server->hostname);
server->hostname = extract_hostname(name);
- spin_unlock(&server->srv_lock);
if (IS_ERR(server->hostname)) {
cifs_dbg(FYI,
"%s: failed to extract hostname from target: %ld\n",
@@ -1177,7 +1175,7 @@
{
struct sockaddr *addr = (struct sockaddr *)&ctx->dstaddr;
- lockdep_assert_held(&cifs_tcp_ses_lock);
+ lockdep_assert_held(&server->srv_lock);
if (ctx->nosharesock)
return 0;
@@ -1196,12 +1194,8 @@
if (!net_eq(cifs_net_ns(server), current->nsproxy->net_ns))
return 0;
- spin_lock(&server->srv_lock);
- if (strcasecmp(server->hostname, ctx->server_hostname)) {
- spin_unlock(&server->srv_lock);
+ if (strcasecmp(server->hostname, ctx->server_hostname))
return 0;
- }
- spin_unlock(&server->srv_lock);
if (!match_address(server, addr,
(struct sockaddr *)&ctx->srcaddr))
@@ -1349,7 +1343,6 @@
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);
################################################################################
! REJECTED PATCH2 HUNKS - could not be compared; manual review needed !
################################################################################
--- b/fs/cifs/cifs_debug.c
+++ b/fs/cifs/cifs_debug.c
@@ -280,6 +280,7 @@
seq_printf(m, "\n%d) ConnectionId: 0x%llx ",
c, server->conn_id);
+ spin_lock(&server->srv_lock);
if (server->hostname)
seq_printf(m, "Hostname: %s ", server->hostname);
#ifdef CONFIG_CIFS_SMB_DIRECT
@@ -282,6 +283,7 @@
if (server->hostname)
seq_printf(m, "Hostname: %s ", server->hostname);
+ spin_unlock(&server->srv_lock);
#ifdef CONFIG_CIFS_SMB_DIRECT
if (!server->rdma)
goto skip_rdma;
--- b/fs/cifs/connect.c
+++ b/fs/cifs/connect.c
@@ -403,6 +403,7 @@
if (server->hostname != target) {
hostname = extract_hostname(target);
if (!IS_ERR(hostname)) {
+ spin_lock(&server->srv_lock);
kfree(server->hostname);
server->hostname = hostname;
} else {
@@ -405,6 +406,7 @@
if (!IS_ERR(hostname)) {
kfree(server->hostname);
server->hostname = hostname;
+ spin_unlock(&server->srv_lock);
} else {
cifs_dbg(FYI, "%s: couldn't extract hostname or address from dfs target: %ld\n",
__func__, PTR_ERR(hostname));
================================================================================
* CONTEXT DIFFERENCES - surrounding code differences between the patches *
================================================================================
--- b/fs/cifs/cifs_debug.c
+++ b/fs/cifs/cifs_debug.c
@@ -614,5 +628,5 @@
atomic_read(&server->smb2slowcmd[j]),
server->hostname, j);
#endif /* STATS2 */
- list_for_each(tmp2, &server->smb_ses_list) {
- ses = list_entry(tmp2, struct cifs_ses,
+ list_for_each_entry(ses, &server->smb_ses_list, smb_ses_list) {
+ list_for_each_entry(tcon, &ses->tcon_list, tcon_list) {
--- b/fs/cifs/connect.c
+++ b/fs/cifs/connect.c
@@ -424,3 +567,2 @@
-#ifdef CONFIG_CIFS_SWN_UPCALL
/* Check witness registrations */
@@ -1181,2 +1408,4 @@
return 0;
+ }
+ spin_unlock(&server->srv_lock);
================================================================================
* ONLY IN PATCH1 - files not modified by patch2 *
================================================================================
--- b/fs/cifs/cifsglob.h
+++ a/fs/cifs/cifsglob.h
@@ -580,7 +580,6 @@
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];
================================================================================
* ONLY IN PATCH2 - files not modified by patch1 *
================================================================================
--- a/fs/cifs/sess.c
+++ b/fs/cifs/sess.c
@@ -159,6 +159,7 @@ cifs_chan_is_iface_active(struct cifs_ses *ses,
/* returns number of channels added */
int cifs_try_adding_channels(struct cifs_sb_info *cifs_sb, struct cifs_ses *ses)
{
+ struct TCP_Server_Info *server = ses->server;
int old_chan_count, new_chan_count;
int left;
int rc = 0;
@@ -178,16 +179,16 @@ int cifs_try_adding_channels(struct cifs_sb_info *cifs_sb, struct cifs_ses *ses)
return 0;
}
- if (ses->server->dialect < SMB30_PROT_ID) {
+ if (server->dialect < SMB30_PROT_ID) {
spin_unlock(&ses->chan_lock);
cifs_dbg(VFS, "multichannel is not supported on this protocol version, use 3.0 or above\n");
return 0;
}
- if (!(ses->server->capabilities & SMB2_GLOBAL_CAP_MULTI_CHANNEL)) {
+ if (!(server->capabilities & SMB2_GLOBAL_CAP_MULTI_CHANNEL)) {
ses->chan_max = 1;
spin_unlock(&ses->chan_lock);
- cifs_dbg(VFS, "server %s does not support multichannel\n", ses->server->hostname);
+ cifs_server_dbg(VFS, "no multichannel support\n");
return 0;
}
spin_unlock(&ses->chan_lock);This is an automated interdiff check for backported commits. |
JIRA PR Check Results4 commit(s) with issues found: Commit
|
|
❌ Validation checks completed with issues View full results: https://github.com/ctrliq/kernel-src-tree/actions/runs/23870172083 |
Contributor
Author
See "Overview" |
Collaborator
|
[WIP] Reviewer Notes:
################################################################################
! REJECTED PATCH2 HUNKS - could not be compared; manual review needed !
################################################################################
--- b/fs/cifs/connect.c
+++ b/fs/cifs/connect.c
@@ -359,7 +359,7 @@
do {
try_to_freeze();
- mutex_lock(&server->srv_mutex);
+ cifs_server_lock(server);
if (!cifs_swn_set_server_dstaddr(server)) {
/* resolve the hostname again to make sure that IP address is up-to-date */This delta : ################################################################################
! REJECTED PATCH2 HUNKS - could not be compared; manual review needed !
################################################################################
--- b/fs/cifs/connect.c
+++ b/fs/cifs/connect.c
@@ -372,7 +372,7 @@
else
rc = generic_ip_connect(server);
if (rc) {
- mutex_unlock(&server->srv_mutex);
+ cifs_server_unlock(server);
cifs_dbg(FYI, "%s: reconnect error %d\n", __func__, rc);
msleep(3000);
} else {This got reordered in ################################################################################
! REJECTED PATCH2 HUNKS - could not be compared; manual review needed !
################################################################################
--- b/fs/cifs/connect.c
+++ b/fs/cifs/connect.c
@@ -510,7 +510,7 @@
server->tcpStatus = CifsNeedNegotiate;
spin_unlock(&cifs_tcp_ses_lock);
cifs_swn_reset_server_dstaddr(server);
- mutex_unlock(&server->srv_mutex);
+ cifs_server_unlock(server);
mod_delayed_work(cifsiod_wq, &server->reconnect, 0);
} while (server->tcpStatus == CifsNeedReconnect);This one is due to yet again ################################################################################
! REJECTED PATCH2 HUNKS - could not be compared; manual review needed !
################################################################################
--- b/fs/cifs/sess.c
+++ b/fs/cifs/sess.c
@@ -1120,7 +1120,7 @@
struct cifs_ses *ses = sess_data->ses;
struct TCP_Server_Info *server = sess_data->server;
- mutex_lock(&server->srv_mutex);
+ cifs_server_lock(server);
if (!server->session_estab) {
if (server->sign) {
server->session_key.response =
@@ -1124,7 +1124,7 @@
kmemdup(ses->auth_key.response,
ses->auth_key.len, GFP_KERNEL);
if (!server->session_key.response) {
- mutex_unlock(&server->srv_mutex);
+ cifs_server_unlock(server);
return -ENOMEM;
}
server->session_key.len =
@@ -1136,7 +1136,7 @@
server->sequence_number = 0x2;
server->session_estab = true;
}
- mutex_unlock(&server->srv_mutex);
+ cifs_server_unlock(server);
cifs_dbg(FYI, "CIFS session established successfully\n");
return 0;This correct due to missing ================================================================================
* ONLY IN PATCH2 - files not modified by patch1 *
================================================================================
--- a/fs/cifs/dfs_cache.c
+++ b/fs/cifs/dfs_cache.c
@@ -1327,9 +1327,9 @@ static bool target_share_equal(struct TCP_Server_Info *server, const char *s1, c
cifs_dbg(VFS, "%s: failed to convert address \'%s\'. skip address matching.\n",
__func__, ip);
} else {
- mutex_lock(&server->srv_mutex);
+ cifs_server_lock(server);
match = cifs_match_ipaddr((struct sockaddr *)&server->dstaddr, &sa);
- mutex_unlock(&server->srv_mutex);
+ cifs_server_unlock(server);
}
kfree(ip);This is expected due to missing |
Collaborator
|
I'm making this one independent as there is a discussion to be had here: ################################################################################
! REJECTED PATCH2 HUNKS - could not be compared; manual review needed !
################################################################################
--- b/fs/cifs/transport.c
+++ b/fs/cifs/transport.c
@@ -1190,7 +1190,7 @@
if ((ses->ses_status == SES_NEW) || (optype & CIFS_NEG_OP) || (optype & CIFS_SESS_OP)) {
spin_unlock(&cifs_tcp_ses_lock);
- mutex_lock(&server->srv_mutex);
+ cifs_server_lock(server);
smb311_update_preauth_hash(ses, server, rqst[0].rq_iov, rqst[0].rq_nvec);
mutex_unlock(&server->srv_mutex);
@@ -1192,7 +1192,7 @@
mutex_lock(&server->srv_mutex);
smb311_update_preauth_hash(ses, server, rqst[0].rq_iov, rqst[0].rq_nvec);
- mutex_unlock(&server->srv_mutex);
+ cifs_server_unlock(server);
spin_lock(&cifs_tcp_ses_lock);
}
@@ -1266,7 +1266,7 @@
.iov_len = resp_iov[0].iov_len
};
spin_unlock(&cifs_tcp_ses_lock);
- mutex_lock(&server->srv_mutex);
+ cifs_server_lock(server);
smb311_update_preauth_hash(ses, server, &iov, 1);
mutex_unlock(&server->srv_mutex);
spin_lock(&cifs_tcp_ses_lock);
@@ -1268,7 +1268,7 @@
spin_unlock(&cifs_tcp_ses_lock);
mutex_lock(&server->srv_mutex);
smb311_update_preauth_hash(ses, server, &iov, 1);
- mutex_unlock(&server->srv_mutex);
+ cifs_server_unlock(server);
spin_lock(&cifs_tcp_ses_lock);
}
spin_unlock(&cifs_tcp_ses_lock);This is because we're missing two commits specifically the But a follow on to that is additional locking fixups |
jallisonciq
approved these changes
Apr 3, 2026
jallisonciq
left a comment
jallisonciq
left a comment
There was a problem hiding this comment.
I think these patches make the code better, and address the specific CVE called out. I don't think they fix everything, but they don't leave the code in a worse place. So on balance, +1 from me.
Collaborator
It appears to me that its turtles all the way down as some things are only addressed as WIWT fixes. Probably works fine for mainline but its hard to handle with backporting to old versions of the kernel. |
|
OK, I just wanted to let you know I've rebuilt this kernel and done a basic SMB3 mount with it, and that cifsfs filesystem still works. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
[LTS 8.6]
Background
From #431:
Commits
Discussion
Overview
The official CVE-2023-53751 fix is just the last commit
cifs: fix potential use-after-free bugs in TCP_Server_Info::hostname. However, the discrepancies between the upstream at the moment of that fix and LTS 8.6 were too large for cherry-picking to make any sense and the changes had to be done manually. This, in turn, prompted tracking of allTCP_Server_Info::hostnameusages, as locking problems require global awareness of the synchronized data usage to be done correctly. This led to the discovery of two other fixes addressing the exact same issue, which is a possible usage ofhostnameafter it was freed during connection re-establishment:cifs: fix race in assemble_neg_contexts(),cifs: protect access of TCP_Server_Info::{dstaddr,hostname}.As such they were included as part of CVE-2023-53751 fix too. The three commits are preceded by
cifs: fix potential deadlock in direct reclaimserving as prerequisite. Coincidentally it also fixes a synchronization issue, although not related to CVE-2023-53751.Threads
While the exact threading layout in
cifsmodule was not established, a key piece of information can be found in commit dca6581:kernel-src-tree/fs/cifs/connect.c
Lines 168 to 169 in dca6581
The
cifs_reconnect()function is a key player in CVE-2023-53751 issue, modifying theTCP_Server_Info::hostnamefield while other threads read it. While it wasn't backported to LTS 8.6 it references a "last patch"most likely the
Fixescommit 2a05137, which wasn't backported either. This suggests that thecifs_reconnect()being used only by a single "cifsd" thread applies to LTS 8.6 as well. This explains why someserver->hostnamereadings on the references list remain (and should remain) unguarded - they all hang on thecifs_reconnect()call tree.Commits discussion
The following sections aim to complement the
upstream-diffinfo, providing additional context, explanations, analysis, rationale, alternative solutions, etc.The entirety of synchronization analysis carried out was based on the assumption that all the corresponding locking / unlocking pairs can be found in the same function. It would be infeasible otherwise. Fortunately the
cifsmodule seems to be following that convention.cifs: fix potential deadlock in direct reclaimThe change seems extensive but it's conceptually simple: instead of using raw
mutex_lock()/mutex_unlock()pairs usecifs_server_lock()/cifs_server_unlock()functions, which are newly introduced wrappers aroundmutex_lock()/mutex_unlock(). What they add is starting / ending theGFP_NOFSallocation scope:kernel-src-tree/include/linux/gfp.h
Line 235 in 5cdfabb
Without it a memory allocation within the mutex span cuold trigger direct reclaim and page writeback to a remote CIFS volume, which in turn would try to acquire the same mutex lock that is already held and cause a deadlock, see https://lore.kernel.org/all/20220523123755.GA13668@axis.com/:
The
cifs_server_lock()/cifs_server_unlock()functions are used by other commits in the proposed solution. Backportingcifs: fix potential deadlock in direct reclaimhelps to avoid the dillemma of either introducing new, potentially deadlocking code paths, or juggling thenofsflag ad hoc for each place the mutex is acquired / freed.cifs: fix race in assemble_neg_contexts()The LTS 8.6 codebase allowed for considerable simplification of the change, explained sufficiently in the
upstream-diff. Apart from that three questions deserve to be addressed.Is the writing of
server->hostnameprotected by theserver->srv_mutexin LTS 8.6?It is. The main area of concern for writing
TCP_Server_Info::hostnameis reconn_set_next_dfs_target(). It's called from cifs_reconnect() (and only there) atkernel-src-tree/fs/cifs/connect.c
Line 328 in a381709
This call is surrounded by
kernel-src-tree/fs/cifs/connect.c
Line 313 in a381709
and one of
kernel-src-tree/fs/cifs/connect.c
Line 355 in a381709
kernel-src-tree/fs/cifs/connect.c
Line 367 in a381709
Writing done by cifs_put_tcp_session() is not protected by the mutex, but neither it is on upstream. This function most likely marks the end of any
TCP_Server_Infoobject usage.Is the LTS 8.6 version of
assemble_neg_contexts()always called outside ofserver->srv_mutexlock?It is. The only
assemble_neg_contexts()call can be found in theSMB2_negotiate()function atkernel-src-tree/fs/cifs/smb2pdu.c
Line 850 in a381709
The
SMB2_negotiate(), in turn, is only called insmb2_negotiate()atkernel-src-tree/fs/cifs/smb2ops.c
Line 349 in a381709
No
srv_mutexlocking to be found in either of these functions. Thesmb2_negotiate()itself is an external operationkernel-src-tree/fs/cifs/smb2ops.c
Line 5129 in a381709
kernel-src-tree/fs/cifs/smb2ops.c
Line 5228 in a381709
kernel-src-tree/fs/cifs/smb2ops.c
Line 5330 in a381709
kernel-src-tree/fs/cifs/smb2ops.c
Line 5443 in a381709
so unlikely to be called with the cifs-specific mutex held.
Does the LTS 8.6 version of
build_netname_ctxt()never lock theserver->srv_mutexlock?It doesn't lock
server->srv_mutex.The call tree:
build_netname_ctxt()(fs/nls/nls_base.c): no locksload_nls_default()(fs/nls/nls_base.c): no locksload_nls()(fs/nls/nls_base.c): no locksfind_nls()(fs/nls/nls_base.c): no lockstry_module_get()(kernel/module.c) function outside ofcifsmodule, no cifs locks expected to be used.try_then_request_module()(include/linux/kmod.h): function outside ofcifsmodule, no cifs locks expected to be used.cifs_strtoUTF16()(fs/cifs/cifs_unicode.c): string-related utility, unlikely to use any locks.cpu_to_le16()(include/linux/byteorder/generic.h) big endian / little endian calculations outside ofcifs.cifs: protect access of TCP_Server_Info::{dstaddr,hostname}The upstream fix addresses the reads of
hostnamein thecifs_tree_connect()function and its subroutinetarget_share_matches_server(). The latter is basically a portion of code extracted fromcifs_tree_connect()in the non-backported commit c88f7dc. That commit involves more changes tocifs_tree_connect(), transforming it from the LTS 8.6 call tree:cifs_tree_connect()(fs/cifs/connect.c)match_target_ip()(fs/cifs/misc.c)into:
cifs_tree_connect()(fs/cifs/connect.c)tree_connect_dfs_target()(fs/cifs/dfs.c)__tree_connect_dfs_target()(fs/cifs/dfs.c)target_share_matches_server()(fs/cifs/dfs.c)match_target_ip()(fs/cifs/misc.c)(with functions narrowed down to those relevant to the fix).
The body of
target_share_matches_server()from before the 39a154f fixkernel-src-tree/fs/cifs/dfs.c
Lines 338 to 347 in 775e44d
corresponds to this LTS 8.6 fragment of the
cifs_tree_connect()function:kernel-src-tree/fs/cifs/connect.c
Lines 4115 to 4126 in 81026fb
The modified fragment of the
__tree_connect_dfs_target()function, in turnkernel-src-tree/fs/cifs/dfs.c
Lines 366 to 397 in 775e44d
corresponds roughly to this LTS 8.6 fragment of
cifs_tree_connect()(encapsulating the previous one):kernel-src-tree/fs/cifs/connect.c
Lines 4098 to 4132 in 81026fb
The upstream fix does not merely wrap the
hostnameusage with theTCP_Server_Info::srv_mutexlock, but also changes the functional semantics by comparingdfs_hostwithserver->hostnamedirectly instead oftcp_host. This functional change was omitted in the backport.From the memory access synchronization perspective it's important to remember that
tcp_hostshares memory withserver->hostname(see definition ofextract_unc_hostname()), so it's not enough to just protect this single linekernel-src-tree/fs/cifs/connect.c
Line 4098 in 81026fb
but all code execution paths starting from the
server->hostnameread to the last usage oftcp_host. This is what motivated moving theextract_unc_hostname()call inside the loop in the backported version. The alternative solutions considered were to eitherserver->hostname, orThe first option was rejected as deviating too much from the upstream fix, at least compared to the eventually proposed solution. There was also no such code pattern found in the
cifsmodule anywhere. The second option was rejected as deviating too much from the upstream as well - there might have been a reason in the upstream to lock thehostnameaccess on a per-iteration basis.Apart from protecting
TCP_Server_Info::hostnamethe upstream fix also puts protection overTCP_Server_Info::dstaddr. While it's not part of the CVE-2023-53751 issue, the change was included for the sake of backport fidelity to the upstream. TheTCP_Server_Info::srv_lockcould not have been used, as it was introduced in the non-backported commit d7d7a66cifs: avoid use of global locks for high contention data. (This lock was eventually included in the next commit cifs: fix potential use-after-free bugs in TCP_Server_Info::hostname, but for a very specific function and could not work in this case.) The usage ofcifs_tcp_ses_lockwas motivated by the existence of this,ciqlts8_6-native, very similar code snippet:kernel-src-tree/fs/cifs/connect.c
Lines 116 to 119 in 81026fb
cifs: fix potential use-after-free bugs in TCP_Server_Info::hostnameThe main problem with backporting this commit was that it was based on using
TCP_Server_Info::srv_locklock not present in LTS 8.6. It was introduced in a large commit d7d7a66cifs: avoid use of global locks for high contention datarewriting most of the spin locks used in thecifsmodule. Backporting it as prerequisite was not an option, as it is too extensive, too conflicting with LTS 8.6 codebase and dealing with too delicate matter.In that commit, among other changes, most of the usages of
cifs_tcp_ses_lockwere offloaded to the newly introducedcifs_tcon::tc_lock,cifs_ses::ses_lockandTCP_Server_Info::srv_lock. At the same time theTCP_Server_Info::srv_lockwas not used in place of any lock other thancifs_tcp_ses_lock(in particular not theGlobalMid_Lock- another de-loaded lock in that commit). As such theTCP_Server_Info::srv_lockbecame a "sub-lock" of the more generalcifs_tcp_ses_lockused before, and in LTS 8.6. Naturally, usingcifs_tcp_ses_lockinstead ofsrv_lockwas considered when backporting 90c49fc.Unfortunately the backported commit locks
srv_lockin thecifs_server_dbg_func()macro, which - through thecifs_server_dbg()wrapper - is used quite extensively in thecifsmodule (78 references ofcifs_server_dbg()found). Changingsrv_locktocifs_tcp_ses_lockrequired tracking all callers of the macro (and their callers, and so on) for whether they don't call it with thecifs_tcp_ses_locklocked already, or a deadlock would occur. As it turned out during the process there was at least one function which required changing, seekernel-src-tree/fs/cifs/smb2transport.c
Lines 94 to 147 in 81026fb
Given the extent of the work required, and the proven necessity of it, the attempt at using
cifs_tcp_ses_lockinstead ofsrv_lockin the backport was abandoned.Instead the
TCP_Server_Info::srv_lockwas introduced as in d7d7a66, with its usages reduced to the extent of the 90c49fc backport only. This was perfectly enough for the CVE-2023-53751 fix to serve its purpose, as the commit was self-contained, in the sense, that:TCP_Server_Info::hostnameexisted in LTS 8.6 before the fix, andTCP_Server_Info::hostnameexisted in LTS 8.6 before the fix.If that was not the case the other protection places would have to be aligned with the backported commit to also use the newly introduced
TCP_Server_Info::srv_lock. (There were, of course,hostnamewriting protections in place already, upon which cifs: protect access of TCP_Server_Info::{dstaddr,hostname} and cifs: fix race in assemble_neg_contexts() backports were built, but they were mutex-based.)Since
TCP_Server_Info::srv_lockdid not exist in LTS 8.6 before the backport it was not necessary to scan all the reversed call trees of the modified functions / macros.Note that there is a bug in
kernel-src-tree/fs/cifs/connect.c
Line 152 in 81026fb
stemming, most likely, from the assumption that
extract_hostname()returns NULL upon error, which is false. It was partially addressed in the (backported) commit 8428817, but the issue ofhostname-reading threads receiving rubbish ifextract_hostname()failed remains. Fixing it was deemed outside of CVE-2023-53751 fix scope though.All
TCP_Server_Info::hostnameusages in LTS 8.6Reference list with a comment about the protection situation for each.
reconn_set_ipaddr_from_hostname()kernel-src-tree/fs/cifs/connect.c
Line 95 in a381709
kernel-src-tree/fs/cifs/connect.c
Line 98 in a381709
kernel-src-tree/fs/cifs/connect.c
Line 105 in a381709
kernel-src-tree/fs/cifs/connect.c
Lines 111 to 112 in a381709
Function called in two places, both within the
cifs_reconnect()call tree - no need for reading protection.kernel-src-tree/fs/cifs/connect.c
Line 159 in a381709
kernel-src-tree/fs/cifs/connect.c
Line 334 in a381709
reconn_set_next_dfs_target()kernel-src-tree/fs/cifs/connect.c
Lines 150 to 152 in a381709
Writing protection addressed in cifs: fix potential use-after-free bugs in TCP_Server_Info::hostname.
cifs_reconnect()kernel-src-tree/fs/cifs/connect.c
Line 243 in a381709
Usage in the
cifs_reconnect()function - no need for reading protection.cifs_echo_request()kernel-src-tree/fs/cifs/connect.c
Lines 422 to 423 in a381709
Issue addressed in cifs: fix potential use-after-free bugs in TCP_Server_Info::hostname (got rid of
hostnameread entirely)match_server()kernel-src-tree/fs/cifs/connect.c
Line 1197 in a381709
Issue addressed in cifs: fix potential use-after-free bugs in TCP_Server_Info::hostname.
cifs_put_tcp_session()kernel-src-tree/fs/cifs/connect.c
Lines 1287 to 1288 in a381709
There is a separate CVE-2025-21673 for that and the upstream fix fa2f990. Did not investigate further, can include it in the PR on request.
cifs_setup_ipc()kernel-src-tree/fs/cifs/connect.c
Line 1515 in a381709
Issue addressed in cifs: fix potential use-after-free bugs in TCP_Server_Info::hostname.
cifs_tree_connect()kernel-src-tree/fs/cifs/connect.c
Line 4084 in a381709
kernel-src-tree/fs/cifs/connect.c
Line 4098 in a381709
kernel-src-tree/fs/cifs/connect.c
Lines 4117 to 4120 in a381709
(memory is shared between
server->hostnameandtcp_host)Issue addressed in cifs: protect access of TCP_Server_Info::{dstaddr,hostname}.
cifs_stats_proc_show()kernel-src-tree/fs/cifs/cifs_debug.c
Lines 613 to 615 in a381709
Issue addressed in cifs: fix potential use-after-free bugs in TCP_Server_Info::hostname.
assemble_neg_contexts()kernel-src-tree/fs/cifs/smb2pdu.c
Lines 552 to 553 in a381709
Issue addressed in cifs: fix race in assemble_neg_contexts().
cifs_get_spnego_key()kernel-src-tree/fs/cifs/cifs_spnego.c
Line 107 in a381709
kernel-src-tree/fs/cifs/cifs_spnego.c
Lines 112 to 118 in a381709
kernel-src-tree/fs/cifs/cifs_spnego.c
Lines 131 to 132 in a381709
Not protected even in the most recent upstream codebase - perhaps not needed.
kernel-src-tree/fs/smb/client/cifs_spnego.c
Lines 83 to 175 in 9147566
cifs_server_dbg_func()kernel-src-tree/fs/cifs/cifs_debug.h
Lines 98 to 110 in a381709
Issue addressed in cifs: fix potential use-after-free bugs in TCP_Server_Info::hostname.
cifs_server_dbg()kernel-src-tree/fs/cifs/cifs_debug.h
Lines 160 to 161 in a381709
Unreachable code, no need for protection (why even there?).
__smb_send_rqst()kernel-src-tree/fs/cifs/transport.c
Lines 454 to 455 in a381709
Already protected, although higher up the callers tree:
__smb_send_rqst()no protectionsmb_send_rqst()(fs/cifs/transport.c) no protectioncompound_send_recv()(fs/cifs/transport.c) protected byserver->srv_mutexcifs_call_async()(fs/cifs/transport.c) protected byserver->srv_mutexsmb_send()(fs/cifs/transport.c) no protectionip_rfc1001_connect()(fs/cifs/connect.c) no protectiongeneric_ip_connect()(fs/cifs/connect.c) no protectioncifs_reconnect()(fs/cifs/connect.c) protected byserver->srv_mutexip_connect()(fs/cifs/connect.c) no protectioncifs_get_tcp_session()(fs/cifs/connect.c) no protection, but in this context it's not needed, seesend_nt_cancel()(fs/cifs/smb1ops.c) protected byserver->srv_mutexSendReceive()(fs/cifs/transport.c) protected byserver->srv_mutexSendReceiveBlockingLock()(fs/cifs/transport.c) protected byserver->srv_mutexwait_for_free_credits()kernel-src-tree/fs/cifs/transport.c
Lines 572 to 573 in a381709
kernel-src-tree/fs/cifs/transport.c
Lines 612 to 615 in a381709
This
hostnameusage doesn't seem to be protected, neither inwait_for_free_credits()nor in any place up the callers tree:wait_for_free_credits()(fs/cifs/transport.c) no protectionwait_for_compound_request()(fs/cifs/transport.c) no protectioncompound_send_recv()(fs/cifs/transport.c) no protection and unlikely to have any up the callers tree, becausesrv_mutexmutex used alreadywait_for_free_request()(fs/cifs/transport.c) no protectioncifs_call_async()(fs/cifs/transport.c) no protection and unlikely to have any up the callers tree, becausesrv_mutexmutex used alreadySendReceive()(fs/cifs/transport.c) no protection and unlikely to have any up the callers tree, becausesrv_mutexmutex used alreadySendReceiveBlockingLock()(fs/cifs/transport.c) no protection and unlikely to have any up the callers tree, becausesrv_mutexmutex used alreadyThese usages aren't protected in the upstream either
kernel-src-tree/fs/smb/client/transport.c
Lines 423 to 575 in 9147566
This might be a bug, especially given that the sibling
TCP_Server_Infofield -tcpStatus- is being protected bysrv_lock, eg.kernel-src-tree/fs/smb/client/transport.c
Lines 471 to 476 in 9147566
No need to outrun the upstream though. Not investigated further.
smb2_add_credits()kernel-src-tree/fs/cifs/smb2ops.c
Lines 87 to 88 in a381709
kernel-src-tree/fs/cifs/smb2ops.c
Lines 137 to 138 in a381709
See wait_for_free_credits().
cifs_get_tcp_session()kernel-src-tree/fs/cifs/connect.c
Lines 1314 to 1318 in a381709
kernel-src-tree/fs/cifs/connect.c
Line 1433 in a381709
No protections in the most recent
kernel-mainlineeither (TCP_Server_Infoobject creation subroutine, perhaps just no need for protection yet).kABI check: passed
Boot test: passed
boot-test.log
Kselftests: passed relative
Reference
kselftests–ciqlts8_6–run1.log
Patch
kselftests–ciqlts8_6-CVE-2023-53751–run1.log
kselftests–ciqlts8_6-CVE-2023-53751–run2.log
Comparison
The tests results for the reference and the patch are the same.
Specific sanity tests: passed
The patched kernel was compiled with these additional options:
The aim was to run at least some of the modified code paths and scan for the possible locking problems. Options
CONFIG_DEBUG_SPINLOCK=yandCONFIG_DEBUG_MUTEXES=yenabled reporting common locking issues. OptionCONFIG_CIFS_DEBUG2=yenabled extensivecifslogging, allowing for easier tracking of the walked code paths. OptionCONFIG_RH_KABI_SIZE_ALIGN_CHECKS=nwas necessary for the mutex debugging option.Additional runtime setup of the LTS 8.6 machine:
See
Documentation/filesystems/cifs/README:kernel-src-tree/Documentation/filesystems/cifs/README
Lines 686 to 689 in 81026fb
Also (see https://wiki.samba.org/index.php/LinuxCIFS_troubleshooting):
The LTS 8.6 machine was set up as the
cifsclient, while the hosting Rocky 9 machine ascifsserver.Server configuration:
Client configuration:
A few moderately large files (~100 MB) were transferred from the server to the client and vice-versa without any issue. Next, a link failure was emulated by shutting down server's network interface to trigger reconnection attempt on the client's side. After a few minutes it was brought back. This resulted in the following logs on the LTS 8.6 machine:
This message
is printed by
kernel-src-tree/fs/cifs/connect.c
Lines 222 to 223 in 81026fb
in the
cifs_reconnect()function addressed by the CVE-2023-53751 fix. Unfortunately it didn't trigger thereconn_set_next_dfs_target()call whereserver->hostnamechange occurs.