[PATCH 1/2] smb: client: hold server ref while queueing reconnect work

[PATCH 1/2] smb: client: hold server ref while queueing reconnect work

[Joseph Qi]

When query_server_interfaces() returns -EOPNOTSUPP on a secondary
channel, smb2_reconnect() calls smb3_update_ses_channels() which for a
channel runs cifs_chan_skip_or_disable().  That clears the channel's
slot in the session and calls cifs_put_tcp_session(), which can drop
the last reference of the channel and tear it down.

smb2_reconnect() then falls through to skip_add_channels and queues
reconnect work on that server, using a server that may already be
freed.  The send path obtained the channel from cifs_pick_channel(),
which does not take a reference, so the session's channel reference is
the only one.

Take our own reference on the channel before calling
smb3_update_ses_channels() and drop it after the reconnect work has
been queued, so the channel cannot be freed while it is still used.

Fixes: f591062bdbf4 ("cifs: handle servers that still advertise multichannel after disabling")
Cc: stable@vger.kernel.org
Signed-off-by: Joseph Qi <joseph.qi@linux.alibaba.com>
---
 fs/smb/client/smb2pdu.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/fs/smb/client/smb2pdu.c b/fs/smb/client/smb2pdu.c
index f85595136ef8a..8e53e9e02df6c 100644
--- a/fs/smb/client/smb2pdu.c
+++ b/fs/smb/client/smb2pdu.c
@@ -253,6 +253,7 @@ smb2_reconnect(__le16 smb2_command, struct cifs_tcon *tcon,
 	struct cifs_ses *ses;
 	int xid;
 	int rc = 0;
+	bool hold_server_ref = false;
 
 	/*
 	 * SMB2s NegProt, SessSetup, Logoff do not have tcon yet so
@@ -470,6 +471,20 @@ smb2_reconnect(__le16 smb2_command, struct cifs_tcon *tcon,
 			 * treat this as server not supporting multichannel
 			 */
 
+			/*
+			 * If server is a secondary channel,
+			 * cifs_chan_skip_or_disable() called below may
+			 * drop its last reference, so hold our own
+			 * reference until the reconnect work below has
+			 * been queued.
+			 */
+			spin_lock(&cifs_tcp_ses_lock);
+			if (server->srv_count) {
+				server->srv_count++;
+				hold_server_ref = true;
+			}
+			spin_unlock(&cifs_tcp_ses_lock);
+
 			rc = smb3_update_ses_channels(ses, server,
 						       from_reconnect,
 						       true /* disable_mchan */);
@@ -498,6 +513,8 @@ smb2_reconnect(__le16 smb2_command, struct cifs_tcon *tcon,
 
 	if (smb2_command != SMB2_INTERNAL_CMD)
 		cifs_queue_server_reconn(server);
+	if (hold_server_ref)
+		cifs_put_tcp_session(server, from_reconnect);
 
 	atomic_inc(&tconInfoReconnectCount);
 out:
-- 
2.39.3

[PATCH 2/2] smb: client: skip NULL channel in __cifs_put_smb_ses()

[Joseph Qi]

cifs_chan_skip_or_disable() clears a secondary channel's slot in the
session (ses->chans[chan_index].server = NULL) without lowering
chan_count when the server stops supporting multichannel.  When the
session is later released, __cifs_put_smb_ses() walks every slot up to
chan_count and calls cifs_put_tcp_session() on it, which dereferences
the server pointer immediately, so a slot left NULL by the skip path
makes the release crash with a NULL pointer dereference.

Clearing a slot without lowering chan_count is older than
cifs_chan_skip_or_disable(): cifs_chan_update_iface() could already
do the same since b54034a73baf9 ("cifs: during reconnect, update
interface if necessary"), and the release loop has been unguarded
since then.  12d1e301bdfd1 ("cifs: do not let cifs_chan_update_iface
deallocate channels") later removed that trigger, but the crash is
reachable through it on older kernels.

cifs_decrease_secondary_channels() already guards the same put with
if (server).  Add the same guard to the release loop so a skipped
channel slot is left alone.

Fixes: b54034a73baf9 ("cifs: during reconnect, update interface if necessary")
Cc: stable@vger.kernel.org
Signed-off-by: Joseph Qi <joseph.qi@linux.alibaba.com>
---
 fs/smb/client/connect.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/fs/smb/client/connect.c b/fs/smb/client/connect.c
index e68fffb2ab2ea..6497b0d54fea7 100644
--- a/fs/smb/client/connect.c
+++ b/fs/smb/client/connect.c
@@ -2200,7 +2200,8 @@ void __cifs_put_smb_ses(struct cifs_ses *ses)
 			kref_put(&ses->chans[i].iface->refcount, release_iface);
 			ses->chans[i].iface = NULL;
 		}
-		cifs_put_tcp_session(ses->chans[i].server, 0);
+		if (ses->chans[i].server)
+			cifs_put_tcp_session(ses->chans[i].server, 0);
 		ses->chans[i].server = NULL;
 	}
 
-- 
2.39.3

Re: [PATCH 2/2] smb: client: skip NULL channel in __cifs_put_smb_ses()

[Paulo Alcantara]

Joseph Qi <joseph.qi@linux.alibaba.com> writes:

> cifs_chan_skip_or_disable() clears a secondary channel's slot in the
> session (ses->chans[chan_index].server = NULL) without lowering
> chan_count when the server stops supporting multichannel.  When the
> session is later released, __cifs_put_smb_ses() walks every slot up to
> chan_count and calls cifs_put_tcp_session() on it, which dereferences
> the server pointer immediately, so a slot left NULL by the skip path
> makes the release crash with a NULL pointer dereference.
>
> Clearing a slot without lowering chan_count is older than
> cifs_chan_skip_or_disable(): cifs_chan_update_iface() could already
> do the same since b54034a73baf9 ("cifs: during reconnect, update
> interface if necessary"), and the release loop has been unguarded
> since then.  12d1e301bdfd1 ("cifs: do not let cifs_chan_update_iface
> deallocate channels") later removed that trigger, but the crash is
> reachable through it on older kernels.
>
> cifs_decrease_secondary_channels() already guards the same put with
> if (server).  Add the same guard to the release loop so a skipped
> channel slot is left alone.
> ...

Applied.

Re: [PATCH 1/2] smb: client: hold server ref while queueing reconnect work

[Paulo Alcantara]

Joseph Qi <joseph.qi@linux.alibaba.com> writes:

> When query_server_interfaces() returns -EOPNOTSUPP on a secondary
> channel, smb2_reconnect() calls smb3_update_ses_channels() which for a
> channel runs cifs_chan_skip_or_disable().  That clears the channel's
> slot in the session and calls cifs_put_tcp_session(), which can drop
> the last reference of the channel and tear it down.
>
> smb2_reconnect() then falls through to skip_add_channels and queues
> reconnect work on that server, using a server that may already be
> freed.  The send path obtained the channel from cifs_pick_channel(),
> which does not take a reference, so the session's channel reference is
> the only one.
>
> Take our own reference on the channel before calling
> smb3_update_ses_channels() and drop it after the reconnect work has
> been queued, so the channel cannot be freed while it is still used.
>
> Fixes: f591062bdbf4 ("cifs: handle servers that still advertise multichannel after disabling")
> Cc: stable@vger.kernel.org
> Signed-off-by: Joseph Qi <joseph.qi@linux.alibaba.com>

Please check sashiko comments

        https://sashiko.dev/#/patchset/20261103091558.2342761-1-joseph.qi%40linux.alibaba.com