backporting 24a9799aa8ef ("smb: client: fix UAF in smb2_reconnect_server()") to older stable series (was: Re: FAILED: patch "[PATCH] smb: client: fix UAF in smb2_reconnect_server()" failed to apply to 6.1-stable tree)

backporting 24a9799aa8ef ("smb: client: fix UAF in smb2_reconnect_server()") to older stable series (was: Re: FAILED: patch "[PATCH] smb: client: fix UAF in smb2_reconnect_server()" failed to apply to 6.1-stable tree)

[Salvatore Bonaccorso]

Hi Paulo, hi Steve,

On Mon, Apr 08, 2024 at 12:19:35PM +0200, gregkh@linuxfoundation.org wrote:
> 
> The patch below does not apply to the 6.1-stable tree.
> If someone wants it applied there, or to any other stable or longterm
> tree, then please email the backport, including the original git commit
> id to <stable@vger.kernel.org>.
> 
> To reproduce the conflict and resubmit, you may use the following commands:
> 
> git fetch https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/ linux-6.1.y
> git checkout FETCH_HEAD
> git cherry-pick -x 24a9799aa8efecd0eb55a75e35f9d8e6400063aa
> # <resolve conflicts, build, test, etc.>
> git commit -s
> git send-email --to '<stable@vger.kernel.org>' --in-reply-to '2024040834-magazine-audience-8aa4@gregkh' --subject-prefix 'PATCH 6.1.y' HEAD^..
> 
> Possible dependencies:
> 
> 24a9799aa8ef ("smb: client: fix UAF in smb2_reconnect_server()")
> 7257bcf3bdc7 ("cifs: cifs_chan_is_iface_active should be called with chan_lock held")
> 27e1fd343f80 ("cifs: after disabling multichannel, mark tcon for reconnect")
> fa1d0508bdd4 ("cifs: account for primary channel in the interface list")
> a6d8fb54a515 ("cifs: distribute channels across interfaces based on speed")
> c37ed2d7d098 ("smb: client: remove extra @chan_count check in __cifs_put_smb_ses()")
> ff7d80a9f271 ("cifs: fix session state transition to avoid use-after-free issue")
> 38c8a9a52082 ("smb: move client and server files to common directory fs/smb")
> 943fb67b0902 ("cifs: missing lock when updating session status")
> bc962159e8e3 ("cifs: avoid race conditions with parallel reconnects")
> 1bcd548d935a ("cifs: prevent data race in cifs_reconnect_tcon()")
> e77978de4765 ("cifs: update ip_addr for ses only for primary chan setup")
> 3c0070f54b31 ("cifs: prevent data race in smb2_reconnect()")
> 05844bd661d9 ("cifs: print last update time for interface list")
> 25cf01b7c920 ("cifs: set correct status of tcon ipc when reconnecting")
> abdb1742a312 ("cifs: get rid of mount options string parsing")
> 9fd29a5bae6e ("cifs: use fs_context for automounts")

In Debian we got a report yhsy in s CIFS (DFS) infrastructure and
after mounting at some point later but reproducible they are able to
trigger within few minutes a system hang with a trace:

CIFS: VFS: \\SOME.SERVER.FQDN cifs_put_smb_ses: Session Logoff failure rc=-11
CIFS: VFS: \\(null) cifs_put_smb_ses: Session Logoff failure rc=-11
list_del corruption, ffff966536fe7800->next is NULL
------------[ cut here ]------------
kernel BUG at lib/list_debug.c:49!
invalid opcode: 0000 [#1] PREEMPT SMP PTI
CPU: 6 PID: 2498151 Comm: kworker/6:9 Tainted: G           OE      6.1.0-23-amd64 #1  Debian 6.1.99-1
Hardware name: Dell Inc. PowerEdge R620/0KCKR5, BIOS 2.9.0 12/06/2019
Workqueue: events delayed_mntput
RIP: 0010:__list_del_entry_valid.cold+0xf/0x6f
Code: c7 c7 88 3c fa a0 e8 90 a0 fe ff 0f 0b 48 c7 c7 60 3c fa a0 e8 82 a0 fe ff 0f 0b 48 89 fe 48 c7 c7 70 3d fa a0 e8 71 a0 fe ff <0f> 0b 48 89 d1 48 c7 c7 90 3e fa a0 48 89 c2 e8 5d a0 fe ff 0f 0b
RSP: 0018:ffffad83a63f7dd0 EFLAGS: 00010246
RAX: 0000000000000033 RBX: ffff966536fe7800 RCX: 0000000000000027
RDX: 0000000000000000 RSI: 0000000000000001 RDI: ffff965e7f8e03a0
RBP: 00000000142d66a6 R08: 0000000000000000 R09: ffffad83a63f7c68
R10: 0000000000000003 R11: ffff966ebff11be0 R12: 00000000fffffff5
R13: ffff966536fe7000 R14: ffff966536fe7020 R15: ffffffffa1770b88
FS:  0000000000000000(0000) GS:ffff965e7f8c0000(0000) knlGS:0000000000000000
CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 00007fe35dbcb7b0 CR3: 0000000f36c10001 CR4: 00000000000606e0
Call Trace:
 <TASK>
 ? __die_body.cold+0x1a/0x1f
 ? die+0x2a/0x50
 ? do_trap+0xc5/0x110
 ? __list_del_entry_valid.cold+0xf/0x6f
 ? do_error_trap+0x6a/0x90
 ? __list_del_entry_valid.cold+0xf/0x6f
 ? exc_invalid_op+0x4c/0x60
 ? __list_del_entry_valid.cold+0xf/0x6f
 ? asm_exc_invalid_op+0x16/0x20
 ? __list_del_entry_valid.cold+0xf/0x6f
 cifs_put_smb_ses+0xbb/0x3e0 [cifs]
 mount_group_release+0x82/0xa0 [cifs]
 cifs_umount+0x88/0xa0 [cifs]
 deactivate_locked_super+0x2f/0xa0
 cleanup_mnt+0xbd/0x150
 delayed_mntput+0x28/0x40
 process_one_work+0x1c7/0x380
 worker_thread+0x4d/0x380
 ? rescuer_thread+0x3a0/0x3a0
 kthread+0xda/0x100
 ? kthread_complete_and_exit+0x20/0x20
 ret_from_fork+0x22/0x30
 </TASK>
Modules linked in: bluetooth jitterentropy_rng drbg ansi_cprng ecdh_generic rfkill ecc overlay isofs cmac nls_utf8 cifs cifs_arc4 cifs_md4 rpcsec_gss_krb5 nfsv4 dns_resolver nfs fscache netfs tls beegfs(OE) rpcrdma rdma_ucm ib_iser rdma_cm iw_cm ib_cm libiscsi scsi_transport_iscsi rdma_rxe ib_uverbs ip6_udp_tunnel udp_tunnel ib_core nft_chain_nat xt_nat nf_nat nf_conntrack nf_defrag_ipv6 nf_defrag_ipv4 nft_compat nf_tables nfnetlink intel_rapl_msr intel_rapl_common sb_edac x86_pkg_temp_thermal intel_powerclamp coretemp kvm_intel ipmi_ssif binfmt_misc kvm irqbypass ghash_clmulni_intel sha512_ssse3 sha512_generic sha256_ssse3 sha1_ssse3 aesni_intel crypto_simd cryptd rapl dcdbas mgag200 intel_cstate joydev evdev drm_shmem_helper intel_uncore iTCO_wdt ipmi_si drm_kms_helper mei_me intel_pmc_bxt ipmi_devintf iTCO_vendor_support pcspkr i2c_algo_bit mei ipmi_msghandler watchdog sg acpi_power_meter button nfsd auth_rpcgss nfs_acl lockd grace sunrpc drm fuse loop efi_pstore configfs
 ip_tables x_tables autofs4 ext4 crc16 mbcache jbd2 dm_mod hid_generic usbhid hid sd_mod t10_pi sr_mod cdrom crc64_rocksoft crc64 crc_t10dif crct10dif_generic ahci libahci crct10dif_pclmul crct10dif_common crc32_pclmul libata ehci_pci bnx2x ehci_hcd megaraid_sas usbcore scsi_mod lpc_ich usb_common mdio libcrc32c crc32c_generic scsi_common crc32c_intel wmi
---[ end trace 0000000000000000 ]---
RIP: 0010:__list_del_entry_valid.cold+0xf/0x6f
Code: c7 c7 88 3c fa a0 e8 90 a0 fe ff 0f 0b 48 c7 c7 60 3c fa a0 e8 82 a0 fe ff 0f 0b 48 89 fe 48 c7 c7 70 3d fa a0 e8 71 a0 fe ff <0f> 0b 48 89 d1 48 c7 c7 90 3e fa a0 48 89 c2 e8 5d a0 fe ff 0f 0b
RSP: 0018:ffffad83a63f7dd0 EFLAGS: 00010246
RAX: 0000000000000033 RBX: ffff966536fe7800 RCX: 0000000000000027
RDX: 0000000000000000 RSI: 0000000000000001 RDI: ffff965e7f8e03a0
RBP: 00000000142d66a6 R08: 0000000000000000 R09: ffffad83a63f7c68
R10: 0000000000000003 R11: ffff966ebff11be0 R12: 00000000fffffff5
R13: ffff966536fe7000 R14: ffff966536fe7020 R15: ffffffffa1770b88
FS:  0000000000000000(0000) GS:ffff965e7f8c0000(0000) knlGS:0000000000000000
CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 00007fe35dbcb7b0 CR3: 0000000f36c10001 CR4: 00000000000606e0
note: kworker/6:9[2498151] exited with preempt_count 1

Michael, did a manual backport of 24a9799aa8ef ("smb: client: fix UAF
in smb2_reconnect_server()") which seems in fact to solve the issue.

Michael, can you please post your backport here for review from Paulo
and Steve?

Regards,
Salvatore

Re: backporting 24a9799aa8ef ("smb: client: fix UAF in smb2_reconnect_server()") to older stable series

[Michael Krause]

[-- Attachment #1: Type: text/plain, Size: 369 bytes --]

Hi *,


On 11/30/24 10:21 AM, Salvatore Bonaccorso wrote:
> Michael, did a manual backport of 24a9799aa8ef ("smb: client: fix UAF
> in smb2_reconnect_server()") which seems in fact to solve the issue.
> 
> Michael, can you please post your backport here for review from Paulo
> and Steve?

Of course, attached.

Now I really hope I didn't screw it up :)

cheers
Michael

[-- Attachment #2: backport.patch --]
[-- Type: text/x-patch, Size: 3407 bytes --]

--- a/fs/smb/client/connect.c	2024-11-22 14:37:35.000000000 +0000
+++ b/fs/smb/client/connect.c	2024-11-30 11:05:53.137339229 +0000
@@ -259,7 +259,13 @@
 
 	spin_lock(&cifs_tcp_ses_lock);
 	list_for_each_entry_safe(ses, nses, &pserver->smb_ses_list, smb_ses_list) {
-		/* check if iface is still active */
+		spin_lock(&ses->ses_lock);
+		if (ses->ses_status == SES_EXITING) {
+			spin_unlock(&ses->ses_lock);
+			continue;
+		}
+		spin_unlock(&ses->ses_lock);
+
 		spin_lock(&ses->chan_lock);
 		if (!cifs_chan_is_iface_active(ses, server)) {
 			spin_unlock(&ses->chan_lock);
@@ -1977,31 +1983,6 @@
 	return rc;
 }
 
-/**
- * cifs_free_ipc - helper to release the session IPC tcon
- * @ses: smb session to unmount the IPC from
- *
- * Needs to be called everytime a session is destroyed.
- *
- * On session close, the IPC is closed and the server must release all tcons of the session.
- * No need to send a tree disconnect here.
- *
- * Besides, it will make the server to not close durable and resilient files on session close, as
- * specified in MS-SMB2 3.3.5.6 Receiving an SMB2 LOGOFF Request.
- */
-static int
-cifs_free_ipc(struct cifs_ses *ses)
-{
-	struct cifs_tcon *tcon = ses->tcon_ipc;
-
-	if (tcon == NULL)
-		return 0;
-
-	tconInfoFree(tcon);
-	ses->tcon_ipc = NULL;
-	return 0;
-}
-
 static struct cifs_ses *
 cifs_find_smb_ses(struct TCP_Server_Info *server, struct smb3_fs_context *ctx)
 {
@@ -2035,35 +2016,44 @@
 {
 	unsigned int rc, xid;
 	unsigned int chan_count;
+ 	bool do_logoff;
+ 	struct cifs_tcon *tcon;
 	struct TCP_Server_Info *server = ses->server;
 
+ 	spin_lock(&cifs_tcp_ses_lock);
 	spin_lock(&ses->ses_lock);
-	if (ses->ses_status == SES_EXITING) {
+	cifs_dbg(FYI, "%s: id=0x%llx ses_count=%d ses_status=%u ipc=%s\n",
+		 __func__, ses->Suid, ses->ses_count, ses->ses_status,
+		 ses->tcon_ipc ? ses->tcon_ipc->tree_name : "none");
+	if (ses->ses_status == SES_EXITING || --ses->ses_count > 0) {
 		spin_unlock(&ses->ses_lock);
+ 		spin_unlock(&cifs_tcp_ses_lock);
 		return;
 	}
-	spin_unlock(&ses->ses_lock);
+ 	/* ses_count can never go negative */
+ 	WARN_ON(ses->ses_count < 0);
 
-	cifs_dbg(FYI, "%s: ses_count=%d\n", __func__, ses->ses_count);
-	cifs_dbg(FYI,
-		 "%s: ses ipc: %s\n", __func__, ses->tcon_ipc ? ses->tcon_ipc->tree_name : "NONE");
-
-	spin_lock(&cifs_tcp_ses_lock);
-	if (--ses->ses_count > 0) {
-		spin_unlock(&cifs_tcp_ses_lock);
-		return;
-	}
+	spin_lock(&ses->chan_lock);
+	cifs_chan_clear_need_reconnect(ses, server);
+	spin_unlock(&ses->chan_lock);
+
+	do_logoff = ses->ses_status == SES_GOOD && server->ops->logoff;
+	ses->ses_status = SES_EXITING;
+	tcon = ses->tcon_ipc;
+	ses->tcon_ipc = NULL;
+ 	spin_unlock(&ses->ses_lock);
 	spin_unlock(&cifs_tcp_ses_lock);
 
-	/* ses_count can never go negative */
-	WARN_ON(ses->ses_count < 0);
-
-	if (ses->ses_status == SES_GOOD)
-		ses->ses_status = SES_EXITING;
-
-	cifs_free_ipc(ses);
-
-	if (ses->ses_status == SES_EXITING && server->ops->logoff) {
+	/*
+	 * On session close, the IPC is closed and the server must release all
+	 * tcons of the session.  No need to send a tree disconnect here.
+	 *
+	 * Besides, it will make the server to not close durable and resilient
+	 * files on session close, as specified in MS-SMB2 3.3.5.6 Receiving an
+	 * SMB2 LOGOFF Request.
+	 */
+	tconInfoFree(tcon);
+	if (do_logoff) {
 		xid = get_xid();
 		rc = server->ops->logoff(xid, ses);
 		if (rc)

Re: backporting 24a9799aa8ef ("smb: client: fix UAF in smb2_reconnect_server()") to older stable series

[Paulo Alcantara]

Michael Krause <mk-debian@galax.is> writes:

> On 11/30/24 10:21 AM, Salvatore Bonaccorso wrote:
>> Michael, did a manual backport of 24a9799aa8ef ("smb: client: fix UAF
>> in smb2_reconnect_server()") which seems in fact to solve the issue.
>> 
>> Michael, can you please post your backport here for review from Paulo
>> and Steve?
>
> Of course, attached.
>
> Now I really hope I didn't screw it up :)

LGTM.  Thanks Michael for the backport.

Re: backporting 24a9799aa8ef ("smb: client: fix UAF in smb2_reconnect_server()") to older stable series

[Salvatore Bonaccorso]

Paulo,

On Tue, Dec 03, 2024 at 10:18:25AM -0300, Paulo Alcantara wrote:
> Michael Krause <mk-debian@galax.is> writes:
> 
> > On 11/30/24 10:21 AM, Salvatore Bonaccorso wrote:
> >> Michael, did a manual backport of 24a9799aa8ef ("smb: client: fix UAF
> >> in smb2_reconnect_server()") which seems in fact to solve the issue.
> >> 
> >> Michael, can you please post your backport here for review from Paulo
> >> and Steve?
> >
> > Of course, attached.
> >
> > Now I really hope I didn't screw it up :)
> 
> LGTM.  Thanks Michael for the backport.

Thanks a lot for the review. So to get it accepted it needs to be
brough into the form which Greg can pick up. Michael can you do that
and add your Signed-off line accordingly?

Regards,
Salvatore

Re: backporting 24a9799aa8ef ("smb: client: fix UAF in smb2_reconnect_server()") to older stable series

[Michael Krause]

On 12/3/24 3:45 PM, Salvatore Bonaccorso wrote:
> Paulo,
> 
> On Tue, Dec 03, 2024 at 10:18:25AM -0300, Paulo Alcantara wrote:
>> Michael Krause <mk-debian@galax.is> writes:
>>
>>> On 11/30/24 10:21 AM, Salvatore Bonaccorso wrote:
>>>> Michael, did a manual backport of 24a9799aa8ef ("smb: client: fix UAF
>>>> in smb2_reconnect_server()") which seems in fact to solve the issue.
>>>>
>>>> Michael, can you please post your backport here for review from Paulo
>>>> and Steve?
>>>
>>> Of course, attached.
>>>
>>> Now I really hope I didn't screw it up :)
>>
>> LGTM.  Thanks Michael for the backport.
> 
> Thanks a lot for the review. So to get it accepted it needs to be
> brough into the form which Greg can pick up. Michael can you do that
> and add your Signed-off line accordingly?
Happy to. Hope this is in the proper format:




 From 411fb6398fe3c3c08a000d717bff189f08d2041c Mon Sep 17 00:00:00 2001
From: Paulo Alcantara <pc@manguebit.com>
Date: Mon, 1 Apr 2024 14:13:10 -0300
Subject: [PATCH] smb: client: fix UAF in smb2_reconnect_server()

commit 24a9799aa8efecd0eb55a75e35f9d8e6400063aa upstream.

The UAF bug is due to smb2_reconnect_server() accessing a session that
is already being teared down by another thread that is executing
__cifs_put_smb_ses().  This can happen when (a) the client has
connection to the server but no session or (b) another thread ends up
setting @ses->ses_status again to something different than
SES_EXITING.

To fix this, we need to make sure to unconditionally set
@ses->ses_status to SES_EXITING and prevent any other threads from
setting a new status while we're still tearing it down.

The following can be reproduced by adding some delay to right after
the ipc is freed in __cifs_put_smb_ses() - which will give
smb2_reconnect_server() worker a chance to run and then accessing
@ses->ipc:

kinit ...
mount.cifs //srv/share /mnt/1 -o sec=krb5,nohandlecache,echo_interval=10
[disconnect srv]
ls /mnt/1 &>/dev/null
sleep 30
kdestroy
[reconnect srv]
sleep 10
umount /mnt/1
...
CIFS: VFS: Verify user has a krb5 ticket and keyutils is installed
CIFS: VFS: \\srv Send error in SessSetup = -126
CIFS: VFS: Verify user has a krb5 ticket and keyutils is installed
CIFS: VFS: \\srv Send error in SessSetup = -126
general protection fault, probably for non-canonical address
0x6b6b6b6b6b6b6b6b: 0000 [#1] PREEMPT SMP NOPTI
CPU: 3 PID: 50 Comm: kworker/3:1 Not tainted 6.9.0-rc2 #1
Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 1.16.3-1.fc39
04/01/2014
Workqueue: cifsiod smb2_reconnect_server [cifs]
RIP: 0010:__list_del_entry_valid_or_report+0x33/0xf0
Code: 4f 08 48 85 d2 74 42 48 85 c9 74 59 48 b8 00 01 00 00 00 00 ad
de 48 39 c2 74 61 48 b8 22 01 00 00 00 00 74 69 <48> 8b 01 48 39 f8 75
7b 48 8b 72 08 48 39 c6 0f 85 88 00 00 00 b8
RSP: 0018:ffffc900001bfd70 EFLAGS: 00010a83
RAX: dead000000000122 RBX: ffff88810da53838 RCX: 6b6b6b6b6b6b6b6b
RDX: 6b6b6b6b6b6b6b6b RSI: ffffffffc02f6878 RDI: ffff88810da53800
RBP: ffff88810da53800 R08: 0000000000000001 R09: 0000000000000000
R10: 0000000000000000 R11: 0000000000000001 R12: ffff88810c064000
R13: 0000000000000001 R14: ffff88810c064000 R15: ffff8881039cc000
FS: 0000000000000000(0000) GS:ffff888157c00000(0000)
knlGS:0000000000000000
CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 00007fe3728b1000 CR3: 000000010caa4000 CR4: 0000000000750ef0
PKRU: 55555554
Call Trace:
  <TASK>
  ? die_addr+0x36/0x90
  ? exc_general_protection+0x1c1/0x3f0
  ? asm_exc_general_protection+0x26/0x30
  ? __list_del_entry_valid_or_report+0x33/0xf0
  __cifs_put_smb_ses+0x1ae/0x500 [cifs]
  smb2_reconnect_server+0x4ed/0x710 [cifs]
  process_one_work+0x205/0x6b0
  worker_thread+0x191/0x360
  ? __pfx_worker_thread+0x10/0x10
  kthread+0xe2/0x110
  ? __pfx_kthread+0x10/0x10
  ret_from_fork+0x34/0x50
  ? __pfx_kthread+0x10/0x10
  ret_from_fork_asm+0x1a/0x30
  </TASK>

Cc: stable@vger.kernel.org
Signed-off-by: Paulo Alcantara (Red Hat) <pc@manguebit.com>
Signed-off-by: Steve French <stfrench@microsoft.com>
[Michael Krause: Naive, manual merge because the 3rd hunk would not
                  apply]
Signed-off-by: Michael Krause <mk-debian@galax.is>
---
  fs/smb/client/connect.c | 80 ++++++++++++++++++-----------------------
  1 file changed, 35 insertions(+), 45 deletions(-)

diff --git a/fs/smb/client/connect.c b/fs/smb/client/connect.c
index 87ce71b39b77..20c50736456a 100644
--- a/fs/smb/client/connect.c
+++ b/fs/smb/client/connect.c
@@ -259,7 +259,13 @@ cifs_mark_tcp_ses_conns_for_reconnect(struct TCP_Server_Info *server,
  
  	spin_lock(&cifs_tcp_ses_lock);
  	list_for_each_entry_safe(ses, nses, &pserver->smb_ses_list, smb_ses_list) {
-		/* check if iface is still active */
+		spin_lock(&ses->ses_lock);
+		if (ses->ses_status == SES_EXITING) {
+			spin_unlock(&ses->ses_lock);
+			continue;
+		}
+		spin_unlock(&ses->ses_lock);
+
  		spin_lock(&ses->chan_lock);
  		if (!cifs_chan_is_iface_active(ses, server)) {
  			spin_unlock(&ses->chan_lock);
@@ -1977,31 +1983,6 @@ cifs_setup_ipc(struct cifs_ses *ses, struct smb3_fs_context *ctx)
  	return rc;
  }
  
-/**
- * cifs_free_ipc - helper to release the session IPC tcon
- * @ses: smb session to unmount the IPC from
- *
- * Needs to be called everytime a session is destroyed.
- *
- * On session close, the IPC is closed and the server must release all tcons of the session.
- * No need to send a tree disconnect here.
- *
- * Besides, it will make the server to not close durable and resilient files on session close, as
- * specified in MS-SMB2 3.3.5.6 Receiving an SMB2 LOGOFF Request.
- */
-static int
-cifs_free_ipc(struct cifs_ses *ses)
-{
-	struct cifs_tcon *tcon = ses->tcon_ipc;
-
-	if (tcon == NULL)
-		return 0;
-
-	tconInfoFree(tcon);
-	ses->tcon_ipc = NULL;
-	return 0;
-}
-
  static struct cifs_ses *
  cifs_find_smb_ses(struct TCP_Server_Info *server, struct smb3_fs_context *ctx)
  {
@@ -2035,35 +2016,44 @@ void cifs_put_smb_ses(struct cifs_ses *ses)
  {
  	unsigned int rc, xid;
  	unsigned int chan_count;
+	bool do_logoff;
+	struct cifs_tcon *tcon;
  	struct TCP_Server_Info *server = ses->server;
  
+	spin_lock(&cifs_tcp_ses_lock);
  	spin_lock(&ses->ses_lock);
-	if (ses->ses_status == SES_EXITING) {
	cifs_dbg(FYI, "%s: id=0x%llx ses_count=%d ses_status=%u ipc=%s\n",
+		 __func__, ses->Suid, ses->ses_count, ses->ses_status,
+		 ses->tcon_ipc ? ses->tcon_ipc->tree_name : "none");
+	if (ses->ses_status == SES_EXITING || --ses->ses_count > 0) {
  		spin_unlock(&ses->ses_lock);
+		spin_unlock(&cifs_tcp_ses_lock);
  		return;
  	}
-	spin_unlock(&ses->ses_lock);
+	/* ses_count can never go negative */
+	WARN_ON(ses->ses_count < 0);
  
-	cifs_dbg(FYI, "%s: ses_count=%d\n", __func__, ses->ses_count);
-	cifs_dbg(FYI,
-		 "%s: ses ipc: %s\n", __func__, ses->tcon_ipc ? ses->tcon_ipc->tree_name : "NONE");
+	spin_lock(&ses->chan_lock);
+	cifs_chan_clear_need_reconnect(ses, server);
+	spin_unlock(&ses->chan_lock);
  
-	spin_lock(&cifs_tcp_ses_lock);
-	if (--ses->ses_count > 0) {
-		spin_unlock(&cifs_tcp_ses_lock);
-		return;
-	}
+	do_logoff = ses->ses_status == SES_GOOD && server->ops->logoff;
+	ses->ses_status = SES_EXITING;
+	tcon = ses->tcon_ipc;
+	ses->tcon_ipc = NULL;
+	spin_unlock(&ses->ses_lock);
  	spin_unlock(&cifs_tcp_ses_lock);
  
-	/* ses_count can never go negative */
-	WARN_ON(ses->ses_count < 0);
-
-	if (ses->ses_status == SES_GOOD)
-		ses->ses_status = SES_EXITING;
-
-	cifs_free_ipc(ses);
-
-	if (ses->ses_status == SES_EXITING && server->ops->logoff) {
+	/*
+	 * On session close, the IPC is closed and the server must release all
+	 * tcons of the session.  No need to send a tree disconnect here.
+	 *
+	 * Besides, it will make the server to not close durable and resilient
+	 * files on session close, as specified in MS-SMB2 3.3.5.6 Receiving an
+	 * SMB2 LOGOFF Request.
+	 */
+	tconInfoFree(tcon);
+	if (do_logoff) {
  		xid = get_xid();
  		rc = server->ops->logoff(xid, ses);
  		if (rc)
-- 
2.45.2

Re: backporting 24a9799aa8ef ("smb: client: fix UAF in smb2_reconnect_server()") to older stable series

[Greg KH]

On Tue, Dec 10, 2024 at 12:05:00AM +0100, Michael Krause wrote:
> On 12/3/24 3:45 PM, Salvatore Bonaccorso wrote:
> > Paulo,
> > 
> > On Tue, Dec 03, 2024 at 10:18:25AM -0300, Paulo Alcantara wrote:
> > > Michael Krause <mk-debian@galax.is> writes:
> > > 
> > > > On 11/30/24 10:21 AM, Salvatore Bonaccorso wrote:
> > > > > Michael, did a manual backport of 24a9799aa8ef ("smb: client: fix UAF
> > > > > in smb2_reconnect_server()") which seems in fact to solve the issue.
> > > > > 
> > > > > Michael, can you please post your backport here for review from Paulo
> > > > > and Steve?
> > > > 
> > > > Of course, attached.
> > > > 
> > > > Now I really hope I didn't screw it up :)
> > > 
> > > LGTM.  Thanks Michael for the backport.
> > 
> > Thanks a lot for the review. So to get it accepted it needs to be
> > brough into the form which Greg can pick up. Michael can you do that
> > and add your Signed-off line accordingly?
> Happy to. Hope this is in the proper format:
> 
> 
> 
> 
> From 411fb6398fe3c3c08a000d717bff189f08d2041c Mon Sep 17 00:00:00 2001
> From: Paulo Alcantara <pc@manguebit.com>
> Date: Mon, 1 Apr 2024 14:13:10 -0300
> Subject: [PATCH] smb: client: fix UAF in smb2_reconnect_server()
> 
> commit 24a9799aa8efecd0eb55a75e35f9d8e6400063aa upstream.
> 
> The UAF bug is due to smb2_reconnect_server() accessing a session that
> is already being teared down by another thread that is executing
> __cifs_put_smb_ses().  This can happen when (a) the client has
> connection to the server but no session or (b) another thread ends up
> setting @ses->ses_status again to something different than
> SES_EXITING.
> 
> To fix this, we need to make sure to unconditionally set
> @ses->ses_status to SES_EXITING and prevent any other threads from
> setting a new status while we're still tearing it down.
> 
> The following can be reproduced by adding some delay to right after
> the ipc is freed in __cifs_put_smb_ses() - which will give
> smb2_reconnect_server() worker a chance to run and then accessing
> @ses->ipc:
> 
> kinit ...
> mount.cifs //srv/share /mnt/1 -o sec=krb5,nohandlecache,echo_interval=10
> [disconnect srv]
> ls /mnt/1 &>/dev/null
> sleep 30
> kdestroy
> [reconnect srv]
> sleep 10
> umount /mnt/1
> ...
> CIFS: VFS: Verify user has a krb5 ticket and keyutils is installed
> CIFS: VFS: \\srv Send error in SessSetup = -126
> CIFS: VFS: Verify user has a krb5 ticket and keyutils is installed
> CIFS: VFS: \\srv Send error in SessSetup = -126
> general protection fault, probably for non-canonical address
> 0x6b6b6b6b6b6b6b6b: 0000 [#1] PREEMPT SMP NOPTI
> CPU: 3 PID: 50 Comm: kworker/3:1 Not tainted 6.9.0-rc2 #1
> Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 1.16.3-1.fc39
> 04/01/2014
> Workqueue: cifsiod smb2_reconnect_server [cifs]
> RIP: 0010:__list_del_entry_valid_or_report+0x33/0xf0
> Code: 4f 08 48 85 d2 74 42 48 85 c9 74 59 48 b8 00 01 00 00 00 00 ad
> de 48 39 c2 74 61 48 b8 22 01 00 00 00 00 74 69 <48> 8b 01 48 39 f8 75
> 7b 48 8b 72 08 48 39 c6 0f 85 88 00 00 00 b8
> RSP: 0018:ffffc900001bfd70 EFLAGS: 00010a83
> RAX: dead000000000122 RBX: ffff88810da53838 RCX: 6b6b6b6b6b6b6b6b
> RDX: 6b6b6b6b6b6b6b6b RSI: ffffffffc02f6878 RDI: ffff88810da53800
> RBP: ffff88810da53800 R08: 0000000000000001 R09: 0000000000000000
> R10: 0000000000000000 R11: 0000000000000001 R12: ffff88810c064000
> R13: 0000000000000001 R14: ffff88810c064000 R15: ffff8881039cc000
> FS: 0000000000000000(0000) GS:ffff888157c00000(0000)
> knlGS:0000000000000000
> CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> CR2: 00007fe3728b1000 CR3: 000000010caa4000 CR4: 0000000000750ef0
> PKRU: 55555554
> Call Trace:
>  <TASK>
>  ? die_addr+0x36/0x90
>  ? exc_general_protection+0x1c1/0x3f0
>  ? asm_exc_general_protection+0x26/0x30
>  ? __list_del_entry_valid_or_report+0x33/0xf0
>  __cifs_put_smb_ses+0x1ae/0x500 [cifs]
>  smb2_reconnect_server+0x4ed/0x710 [cifs]
>  process_one_work+0x205/0x6b0
>  worker_thread+0x191/0x360
>  ? __pfx_worker_thread+0x10/0x10
>  kthread+0xe2/0x110
>  ? __pfx_kthread+0x10/0x10
>  ret_from_fork+0x34/0x50
>  ? __pfx_kthread+0x10/0x10
>  ret_from_fork_asm+0x1a/0x30
>  </TASK>
> 
> Cc: stable@vger.kernel.org
> Signed-off-by: Paulo Alcantara (Red Hat) <pc@manguebit.com>
> Signed-off-by: Steve French <stfrench@microsoft.com>
> [Michael Krause: Naive, manual merge because the 3rd hunk would not
>                  apply]
> Signed-off-by: Michael Krause <mk-debian@galax.is>
> ---
>  fs/smb/client/connect.c | 80 ++++++++++++++++++-----------------------
>  1 file changed, 35 insertions(+), 45 deletions(-)

What kernel(s) is this commit supposed to be for?

thanks,

greg k-h

Re: backporting 24a9799aa8ef ("smb: client: fix UAF in smb2_reconnect_server()") to older stable series

[Salvatore Bonaccorso]

Hi Greg,

On Tue, Dec 10, 2024 at 09:51:58AM +0100, Greg KH wrote:
> On Tue, Dec 10, 2024 at 12:05:00AM +0100, Michael Krause wrote:
> > On 12/3/24 3:45 PM, Salvatore Bonaccorso wrote:
> > > Paulo,
> > > 
> > > On Tue, Dec 03, 2024 at 10:18:25AM -0300, Paulo Alcantara wrote:
> > > > Michael Krause <mk-debian@galax.is> writes:
> > > > 
> > > > > On 11/30/24 10:21 AM, Salvatore Bonaccorso wrote:
> > > > > > Michael, did a manual backport of 24a9799aa8ef ("smb: client: fix UAF
> > > > > > in smb2_reconnect_server()") which seems in fact to solve the issue.
> > > > > > 
> > > > > > Michael, can you please post your backport here for review from Paulo
> > > > > > and Steve?
> > > > > 
> > > > > Of course, attached.
> > > > > 
> > > > > Now I really hope I didn't screw it up :)
> > > > 
> > > > LGTM.  Thanks Michael for the backport.
> > > 
> > > Thanks a lot for the review. So to get it accepted it needs to be
> > > brough into the form which Greg can pick up. Michael can you do that
> > > and add your Signed-off line accordingly?
> > Happy to. Hope this is in the proper format:
> > 
> > 
> > 
> > 
> > From 411fb6398fe3c3c08a000d717bff189f08d2041c Mon Sep 17 00:00:00 2001
> > From: Paulo Alcantara <pc@manguebit.com>
> > Date: Mon, 1 Apr 2024 14:13:10 -0300
> > Subject: [PATCH] smb: client: fix UAF in smb2_reconnect_server()
> > 
> > commit 24a9799aa8efecd0eb55a75e35f9d8e6400063aa upstream.
> > 
> > The UAF bug is due to smb2_reconnect_server() accessing a session that
> > is already being teared down by another thread that is executing
> > __cifs_put_smb_ses().  This can happen when (a) the client has
> > connection to the server but no session or (b) another thread ends up
> > setting @ses->ses_status again to something different than
> > SES_EXITING.
> > 
> > To fix this, we need to make sure to unconditionally set
> > @ses->ses_status to SES_EXITING and prevent any other threads from
> > setting a new status while we're still tearing it down.
> > 
> > The following can be reproduced by adding some delay to right after
> > the ipc is freed in __cifs_put_smb_ses() - which will give
> > smb2_reconnect_server() worker a chance to run and then accessing
> > @ses->ipc:
> > 
> > kinit ...
> > mount.cifs //srv/share /mnt/1 -o sec=krb5,nohandlecache,echo_interval=10
> > [disconnect srv]
> > ls /mnt/1 &>/dev/null
> > sleep 30
> > kdestroy
> > [reconnect srv]
> > sleep 10
> > umount /mnt/1
> > ...
> > CIFS: VFS: Verify user has a krb5 ticket and keyutils is installed
> > CIFS: VFS: \\srv Send error in SessSetup = -126
> > CIFS: VFS: Verify user has a krb5 ticket and keyutils is installed
> > CIFS: VFS: \\srv Send error in SessSetup = -126
> > general protection fault, probably for non-canonical address
> > 0x6b6b6b6b6b6b6b6b: 0000 [#1] PREEMPT SMP NOPTI
> > CPU: 3 PID: 50 Comm: kworker/3:1 Not tainted 6.9.0-rc2 #1
> > Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 1.16.3-1.fc39
> > 04/01/2014
> > Workqueue: cifsiod smb2_reconnect_server [cifs]
> > RIP: 0010:__list_del_entry_valid_or_report+0x33/0xf0
> > Code: 4f 08 48 85 d2 74 42 48 85 c9 74 59 48 b8 00 01 00 00 00 00 ad
> > de 48 39 c2 74 61 48 b8 22 01 00 00 00 00 74 69 <48> 8b 01 48 39 f8 75
> > 7b 48 8b 72 08 48 39 c6 0f 85 88 00 00 00 b8
> > RSP: 0018:ffffc900001bfd70 EFLAGS: 00010a83
> > RAX: dead000000000122 RBX: ffff88810da53838 RCX: 6b6b6b6b6b6b6b6b
> > RDX: 6b6b6b6b6b6b6b6b RSI: ffffffffc02f6878 RDI: ffff88810da53800
> > RBP: ffff88810da53800 R08: 0000000000000001 R09: 0000000000000000
> > R10: 0000000000000000 R11: 0000000000000001 R12: ffff88810c064000
> > R13: 0000000000000001 R14: ffff88810c064000 R15: ffff8881039cc000
> > FS: 0000000000000000(0000) GS:ffff888157c00000(0000)
> > knlGS:0000000000000000
> > CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> > CR2: 00007fe3728b1000 CR3: 000000010caa4000 CR4: 0000000000750ef0
> > PKRU: 55555554
> > Call Trace:
> >  <TASK>
> >  ? die_addr+0x36/0x90
> >  ? exc_general_protection+0x1c1/0x3f0
> >  ? asm_exc_general_protection+0x26/0x30
> >  ? __list_del_entry_valid_or_report+0x33/0xf0
> >  __cifs_put_smb_ses+0x1ae/0x500 [cifs]
> >  smb2_reconnect_server+0x4ed/0x710 [cifs]
> >  process_one_work+0x205/0x6b0
> >  worker_thread+0x191/0x360
> >  ? __pfx_worker_thread+0x10/0x10
> >  kthread+0xe2/0x110
> >  ? __pfx_kthread+0x10/0x10
> >  ret_from_fork+0x34/0x50
> >  ? __pfx_kthread+0x10/0x10
> >  ret_from_fork_asm+0x1a/0x30
> >  </TASK>
> > 
> > Cc: stable@vger.kernel.org
> > Signed-off-by: Paulo Alcantara (Red Hat) <pc@manguebit.com>
> > Signed-off-by: Steve French <stfrench@microsoft.com>
> > [Michael Krause: Naive, manual merge because the 3rd hunk would not
> >                  apply]
> > Signed-off-by: Michael Krause <mk-debian@galax.is>
> > ---
> >  fs/smb/client/connect.c | 80 ++++++++++++++++++-----------------------
> >  1 file changed, 35 insertions(+), 45 deletions(-)
> 
> What kernel(s) is this commit supposed to be for?

6.1.y (at least, possibly older, but the reporter did test on 6.1.y
for Debian, context from https://bugs.debian.org/1088733). Upper
stable series have already the commit in 6.6.29, 6.8.5.

Regards,
Salvatore

Re: backporting 24a9799aa8ef ("smb: client: fix UAF in smb2_reconnect_server()") to older stable series

[Greg KH]

On Tue, Dec 10, 2024 at 12:05:00AM +0100, Michael Krause wrote:
> On 12/3/24 3:45 PM, Salvatore Bonaccorso wrote:
> > Paulo,
> > 
> > On Tue, Dec 03, 2024 at 10:18:25AM -0300, Paulo Alcantara wrote:
> > > Michael Krause <mk-debian@galax.is> writes:
> > > 
> > > > On 11/30/24 10:21 AM, Salvatore Bonaccorso wrote:
> > > > > Michael, did a manual backport of 24a9799aa8ef ("smb: client: fix UAF
> > > > > in smb2_reconnect_server()") which seems in fact to solve the issue.
> > > > > 
> > > > > Michael, can you please post your backport here for review from Paulo
> > > > > and Steve?
> > > > 
> > > > Of course, attached.
> > > > 
> > > > Now I really hope I didn't screw it up :)
> > > 
> > > LGTM.  Thanks Michael for the backport.
> > 
> > Thanks a lot for the review. So to get it accepted it needs to be
> > brough into the form which Greg can pick up. Michael can you do that
> > and add your Signed-off line accordingly?
> Happy to. Hope this is in the proper format:

It's corrupted somehow:

patching file fs/smb/client/connect.c
patch: **** malformed patch at line 202:  		if (rc)


Can you resend it or attach it?

thanks,

greg k-h

Re: backporting 24a9799aa8ef ("smb: client: fix UAF in smb2_reconnect_server()") to older stable series

[Michael Krause]

[-- Attachment #1: Type: text/plain, Size: 8417 bytes --]

On 12/12/24 1:26 PM, Greg KH wrote:
> On Tue, Dec 10, 2024 at 12:05:00AM +0100, Michael Krause wrote:
>> On 12/3/24 3:45 PM, Salvatore Bonaccorso wrote:
>>> Paulo,
>>>
>>> On Tue, Dec 03, 2024 at 10:18:25AM -0300, Paulo Alcantara wrote:
>>>> Michael Krause <mk-debian@galax.is> writes:
>>>>
>>>>> On 11/30/24 10:21 AM, Salvatore Bonaccorso wrote:
>>>>>> Michael, did a manual backport of 24a9799aa8ef ("smb: client: fix UAF
>>>>>> in smb2_reconnect_server()") which seems in fact to solve the issue.
>>>>>>
>>>>>> Michael, can you please post your backport here for review from Paulo
>>>>>> and Steve?
>>>>>
>>>>> Of course, attached.
>>>>>
>>>>> Now I really hope I didn't screw it up :)
>>>>
>>>> LGTM.  Thanks Michael for the backport.
>>>
>>> Thanks a lot for the review. So to get it accepted it needs to be
>>> brough into the form which Greg can pick up. Michael can you do that
>>> and add your Signed-off line accordingly?
>> Happy to. Hope this is in the proper format:
> 
> It's corrupted somehow:
> 
> patching file fs/smb/client/connect.c
> patch: **** malformed patch at line 202:  		if (rc)
> 
> 
> Can you resend it or attach it?
> 
> thanks,
> 
> greg k-h

Ugh, how embarrassing. I'm sorry, I "fixed" some minor whitespace issue directly in the patch and apparently did something wrong.

I redid the white space fix before diffing again and attach and inline the new version. The chunks are a bit alternated to the earlier version now unfortunately. This one applies..






 From 411fb6398fe3c3c08a000d717bff189f08d2041c Mon Sep 17 00:00:00 2001
From: Paulo Alcantara <pc@manguebit.com>
Date: Mon, 1 Apr 2024 14:13:10 -0300
Subject: [PATCH] smb: client: fix UAF in smb2_reconnect_server()

commit 24a9799aa8efecd0eb55a75e35f9d8e6400063aa upstream.

The UAF bug is due to smb2_reconnect_server() accessing a session that
is already being teared down by another thread that is executing
__cifs_put_smb_ses().  This can happen when (a) the client has
connection to the server but no session or (b) another thread ends up
setting @ses->ses_status again to something different than
SES_EXITING.

To fix this, we need to make sure to unconditionally set
@ses->ses_status to SES_EXITING and prevent any other threads from
setting a new status while we're still tearing it down.

The following can be reproduced by adding some delay to right after
the ipc is freed in __cifs_put_smb_ses() - which will give
smb2_reconnect_server() worker a chance to run and then accessing
@ses->ipc:

kinit ...
mount.cifs //srv/share /mnt/1 -o sec=krb5,nohandlecache,echo_interval=10
[disconnect srv]
ls /mnt/1 &>/dev/null
sleep 30
kdestroy
[reconnect srv]
sleep 10
umount /mnt/1
...
CIFS: VFS: Verify user has a krb5 ticket and keyutils is installed
CIFS: VFS: \\srv Send error in SessSetup = -126
CIFS: VFS: Verify user has a krb5 ticket and keyutils is installed
CIFS: VFS: \\srv Send error in SessSetup = -126
general protection fault, probably for non-canonical address
0x6b6b6b6b6b6b6b6b: 0000 [#1] PREEMPT SMP NOPTI
CPU: 3 PID: 50 Comm: kworker/3:1 Not tainted 6.9.0-rc2 #1
Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 1.16.3-1.fc39
04/01/2014
Workqueue: cifsiod smb2_reconnect_server [cifs]
RIP: 0010:__list_del_entry_valid_or_report+0x33/0xf0
Code: 4f 08 48 85 d2 74 42 48 85 c9 74 59 48 b8 00 01 00 00 00 00 ad
de 48 39 c2 74 61 48 b8 22 01 00 00 00 00 74 69 <48> 8b 01 48 39 f8 75
7b 48 8b 72 08 48 39 c6 0f 85 88 00 00 00 b8
RSP: 0018:ffffc900001bfd70 EFLAGS: 00010a83
RAX: dead000000000122 RBX: ffff88810da53838 RCX: 6b6b6b6b6b6b6b6b
RDX: 6b6b6b6b6b6b6b6b RSI: ffffffffc02f6878 RDI: ffff88810da53800
RBP: ffff88810da53800 R08: 0000000000000001 R09: 0000000000000000
R10: 0000000000000000 R11: 0000000000000001 R12: ffff88810c064000
R13: 0000000000000001 R14: ffff88810c064000 R15: ffff8881039cc000
FS: 0000000000000000(0000) GS:ffff888157c00000(0000)
knlGS:0000000000000000
CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 00007fe3728b1000 CR3: 000000010caa4000 CR4: 0000000000750ef0
PKRU: 55555554
Call Trace:
  <TASK>
  ? die_addr+0x36/0x90
  ? exc_general_protection+0x1c1/0x3f0
  ? asm_exc_general_protection+0x26/0x30
  ? __list_del_entry_valid_or_report+0x33/0xf0
  __cifs_put_smb_ses+0x1ae/0x500 [cifs]
  smb2_reconnect_server+0x4ed/0x710 [cifs]
  process_one_work+0x205/0x6b0
  worker_thread+0x191/0x360
  ? __pfx_worker_thread+0x10/0x10
  kthread+0xe2/0x110
  ? __pfx_kthread+0x10/0x10
  ret_from_fork+0x34/0x50
  ? __pfx_kthread+0x10/0x10
  ret_from_fork_asm+0x1a/0x30
  </TASK>

Cc: stable@vger.kernel.org
Signed-off-by: Paulo Alcantara (Red Hat) <pc@manguebit.com>
Signed-off-by: Steve French <stfrench@microsoft.com>
[Michael Krause: Naive, manual merge because the 3rd hunk would not
                  apply.]
Signed-off-by: Michael Krause <mk-debian@galax.is>
---
  fs/smb/client/connect.c | 80 ++++++++++++++++++-----------------------
  1 file changed, 35 insertions(+), 45 deletions(-)

diff --git a/fs/smb/client/connect.c b/fs/smb/client/connect.c
index 87ce71b39b77..20c50736456a 100644
--- a/fs/smb/client/connect.c
+++ b/fs/smb/client/connect.c
@@ -259,7 +259,13 @@ cifs_mark_tcp_ses_conns_for_reconnect(st
  
  	spin_lock(&cifs_tcp_ses_lock);
  	list_for_each_entry_safe(ses, nses, &pserver->smb_ses_list, smb_ses_list) {
-		/* check if iface is still active */
+		spin_lock(&ses->ses_lock);
+		if (ses->ses_status == SES_EXITING) {
+			spin_unlock(&ses->ses_lock);
+			continue;
+		}
+		spin_unlock(&ses->ses_lock);
+
  		spin_lock(&ses->chan_lock);
  		if (!cifs_chan_is_iface_active(ses, server)) {
  			spin_unlock(&ses->chan_lock);
@@ -1977,31 +1983,6 @@ out:
  	return rc;
  }
  
-/**
- * cifs_free_ipc - helper to release the session IPC tcon
- * @ses: smb session to unmount the IPC from
- *
- * Needs to be called everytime a session is destroyed.
- *
- * On session close, the IPC is closed and the server must release all tcons of the session.
- * No need to send a tree disconnect here.
- *
- * Besides, it will make the server to not close durable and resilient files on session close, as
- * specified in MS-SMB2 3.3.5.6 Receiving an SMB2 LOGOFF Request.
- */
-static int
-cifs_free_ipc(struct cifs_ses *ses)
-{
-	struct cifs_tcon *tcon = ses->tcon_ipc;
-
-	if (tcon == NULL)
-		return 0;
-
-	tconInfoFree(tcon);
-	ses->tcon_ipc = NULL;
-	return 0;
-}
-
  static struct cifs_ses *
  cifs_find_smb_ses(struct TCP_Server_Info *server, struct smb3_fs_context *ctx)
  {
@@ -2035,35 +2016,44 @@ void cifs_put_smb_ses(struct cifs_ses *s
  {
  	unsigned int rc, xid;
  	unsigned int chan_count;
+	bool do_logoff;
+	struct cifs_tcon *tcon;
  	struct TCP_Server_Info *server = ses->server;
  
+	spin_lock(&cifs_tcp_ses_lock);
  	spin_lock(&ses->ses_lock);
-	if (ses->ses_status == SES_EXITING) {
+	cifs_dbg(FYI, "%s: id=0x%llx ses_count=%d ses_status=%u ipc=%s\n",
+		 __func__, ses->Suid, ses->ses_count, ses->ses_status,
+		 ses->tcon_ipc ? ses->tcon_ipc->tree_name : "none");
+	if (ses->ses_status == SES_EXITING || --ses->ses_count > 0) {
  		spin_unlock(&ses->ses_lock);
-		return;
-	}
-	spin_unlock(&ses->ses_lock);
-
-	cifs_dbg(FYI, "%s: ses_count=%d\n", __func__, ses->ses_count);
-	cifs_dbg(FYI,
-		 "%s: ses ipc: %s\n", __func__, ses->tcon_ipc ? ses->tcon_ipc->tree_name : "NONE");
-
-	spin_lock(&cifs_tcp_ses_lock);
-	if (--ses->ses_count > 0) {
  		spin_unlock(&cifs_tcp_ses_lock);
  		return;
  	}
-	spin_unlock(&cifs_tcp_ses_lock);
-
  	/* ses_count can never go negative */
  	WARN_ON(ses->ses_count < 0);
  
-	if (ses->ses_status == SES_GOOD)
-		ses->ses_status = SES_EXITING;
-
-	cifs_free_ipc(ses);
+	spin_lock(&ses->chan_lock);
+	cifs_chan_clear_need_reconnect(ses, server);
+	spin_unlock(&ses->chan_lock);
+
+	do_logoff = ses->ses_status == SES_GOOD && server->ops->logoff;
+	ses->ses_status = SES_EXITING;
+	tcon = ses->tcon_ipc;
+	ses->tcon_ipc = NULL;
+ 	spin_unlock(&ses->ses_lock);
+	spin_unlock(&cifs_tcp_ses_lock);
  
-	if (ses->ses_status == SES_EXITING && server->ops->logoff) {
+	/*
+	 * On session close, the IPC is closed and the server must release all
+	 * tcons of the session.  No need to send a tree disconnect here.
+	 *
+	 * Besides, it will make the server to not close durable and resilient
+	 * files on session close, as specified in MS-SMB2 3.3.5.6 Receiving an
+	 * SMB2 LOGOFF Request.
+	 */
+	tconInfoFree(tcon);
+	if (do_logoff) {
  		xid = get_xid();
  		rc = server->ops->logoff(xid, ses);
  		if (rc)
-- 
2.45.2

[-- Attachment #2: backport-6.1-smb-client-fix-UAF-in-smb2_reconnect_server.v2.patch --]
[-- Type: text/x-patch, Size: 6877 bytes --]

From 411fb6398fe3c3c08a000d717bff189f08d2041c Mon Sep 17 00:00:00 2001
From: Paulo Alcantara <pc@manguebit.com>
Date: Mon, 1 Apr 2024 14:13:10 -0300
Subject: [PATCH] smb: client: fix UAF in smb2_reconnect_server()

commit 24a9799aa8efecd0eb55a75e35f9d8e6400063aa upstream.

The UAF bug is due to smb2_reconnect_server() accessing a session that
is already being teared down by another thread that is executing
__cifs_put_smb_ses().  This can happen when (a) the client has
connection to the server but no session or (b) another thread ends up
setting @ses->ses_status again to something different than
SES_EXITING.

To fix this, we need to make sure to unconditionally set
@ses->ses_status to SES_EXITING and prevent any other threads from
setting a new status while we're still tearing it down.

The following can be reproduced by adding some delay to right after
the ipc is freed in __cifs_put_smb_ses() - which will give
smb2_reconnect_server() worker a chance to run and then accessing
@ses->ipc:

kinit ...
mount.cifs //srv/share /mnt/1 -o sec=krb5,nohandlecache,echo_interval=10
[disconnect srv]
ls /mnt/1 &>/dev/null
sleep 30
kdestroy
[reconnect srv]
sleep 10
umount /mnt/1
...
CIFS: VFS: Verify user has a krb5 ticket and keyutils is installed
CIFS: VFS: \\srv Send error in SessSetup = -126
CIFS: VFS: Verify user has a krb5 ticket and keyutils is installed
CIFS: VFS: \\srv Send error in SessSetup = -126
general protection fault, probably for non-canonical address
0x6b6b6b6b6b6b6b6b: 0000 [#1] PREEMPT SMP NOPTI
CPU: 3 PID: 50 Comm: kworker/3:1 Not tainted 6.9.0-rc2 #1
Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 1.16.3-1.fc39
04/01/2014
Workqueue: cifsiod smb2_reconnect_server [cifs]
RIP: 0010:__list_del_entry_valid_or_report+0x33/0xf0
Code: 4f 08 48 85 d2 74 42 48 85 c9 74 59 48 b8 00 01 00 00 00 00 ad
de 48 39 c2 74 61 48 b8 22 01 00 00 00 00 74 69 <48> 8b 01 48 39 f8 75
7b 48 8b 72 08 48 39 c6 0f 85 88 00 00 00 b8
RSP: 0018:ffffc900001bfd70 EFLAGS: 00010a83
RAX: dead000000000122 RBX: ffff88810da53838 RCX: 6b6b6b6b6b6b6b6b
RDX: 6b6b6b6b6b6b6b6b RSI: ffffffffc02f6878 RDI: ffff88810da53800
RBP: ffff88810da53800 R08: 0000000000000001 R09: 0000000000000000
R10: 0000000000000000 R11: 0000000000000001 R12: ffff88810c064000
R13: 0000000000000001 R14: ffff88810c064000 R15: ffff8881039cc000
FS: 0000000000000000(0000) GS:ffff888157c00000(0000)
knlGS:0000000000000000
CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 00007fe3728b1000 CR3: 000000010caa4000 CR4: 0000000000750ef0
PKRU: 55555554
Call Trace:
 <TASK>
 ? die_addr+0x36/0x90
 ? exc_general_protection+0x1c1/0x3f0
 ? asm_exc_general_protection+0x26/0x30
 ? __list_del_entry_valid_or_report+0x33/0xf0
 __cifs_put_smb_ses+0x1ae/0x500 [cifs]
 smb2_reconnect_server+0x4ed/0x710 [cifs]
 process_one_work+0x205/0x6b0
 worker_thread+0x191/0x360
 ? __pfx_worker_thread+0x10/0x10
 kthread+0xe2/0x110
 ? __pfx_kthread+0x10/0x10
 ret_from_fork+0x34/0x50
 ? __pfx_kthread+0x10/0x10
 ret_from_fork_asm+0x1a/0x30
 </TASK>

Cc: stable@vger.kernel.org
Signed-off-by: Paulo Alcantara (Red Hat) <pc@manguebit.com>
Signed-off-by: Steve French <stfrench@microsoft.com>
[Michael Krause: Naive, manual merge because the 3rd hunk would not
                 apply.]
Signed-off-by: Michael Krause <mk-debian@galax.is>
---
 fs/smb/client/connect.c | 80 ++++++++++++++++++-----------------------
 1 file changed, 35 insertions(+), 45 deletions(-)

diff --git a/fs/smb/client/connect.c b/fs/smb/client/connect.c
index 87ce71b39b77..20c50736456a 100644
--- a/fs/smb/client/connect.c
+++ b/fs/smb/client/connect.c
@@ -259,7 +259,13 @@ cifs_mark_tcp_ses_conns_for_reconnect(st
 
 	spin_lock(&cifs_tcp_ses_lock);
 	list_for_each_entry_safe(ses, nses, &pserver->smb_ses_list, smb_ses_list) {
-		/* check if iface is still active */
+		spin_lock(&ses->ses_lock);
+		if (ses->ses_status == SES_EXITING) {
+			spin_unlock(&ses->ses_lock);
+			continue;
+		}
+		spin_unlock(&ses->ses_lock);
+
 		spin_lock(&ses->chan_lock);
 		if (!cifs_chan_is_iface_active(ses, server)) {
 			spin_unlock(&ses->chan_lock);
@@ -1977,31 +1983,6 @@ out:
 	return rc;
 }
 
-/**
- * cifs_free_ipc - helper to release the session IPC tcon
- * @ses: smb session to unmount the IPC from
- *
- * Needs to be called everytime a session is destroyed.
- *
- * On session close, the IPC is closed and the server must release all tcons of the session.
- * No need to send a tree disconnect here.
- *
- * Besides, it will make the server to not close durable and resilient files on session close, as
- * specified in MS-SMB2 3.3.5.6 Receiving an SMB2 LOGOFF Request.
- */
-static int
-cifs_free_ipc(struct cifs_ses *ses)
-{
-	struct cifs_tcon *tcon = ses->tcon_ipc;
-
-	if (tcon == NULL)
-		return 0;
-
-	tconInfoFree(tcon);
-	ses->tcon_ipc = NULL;
-	return 0;
-}
-
 static struct cifs_ses *
 cifs_find_smb_ses(struct TCP_Server_Info *server, struct smb3_fs_context *ctx)
 {
@@ -2035,35 +2016,44 @@ void cifs_put_smb_ses(struct cifs_ses *s
 {
 	unsigned int rc, xid;
 	unsigned int chan_count;
+	bool do_logoff;
+	struct cifs_tcon *tcon;
 	struct TCP_Server_Info *server = ses->server;
 
+	spin_lock(&cifs_tcp_ses_lock);
 	spin_lock(&ses->ses_lock);
-	if (ses->ses_status == SES_EXITING) {
+	cifs_dbg(FYI, "%s: id=0x%llx ses_count=%d ses_status=%u ipc=%s\n",
+		 __func__, ses->Suid, ses->ses_count, ses->ses_status,
+		 ses->tcon_ipc ? ses->tcon_ipc->tree_name : "none");
+	if (ses->ses_status == SES_EXITING || --ses->ses_count > 0) {
 		spin_unlock(&ses->ses_lock);
-		return;
-	}
-	spin_unlock(&ses->ses_lock);
-
-	cifs_dbg(FYI, "%s: ses_count=%d\n", __func__, ses->ses_count);
-	cifs_dbg(FYI,
-		 "%s: ses ipc: %s\n", __func__, ses->tcon_ipc ? ses->tcon_ipc->tree_name : "NONE");
-
-	spin_lock(&cifs_tcp_ses_lock);
-	if (--ses->ses_count > 0) {
 		spin_unlock(&cifs_tcp_ses_lock);
 		return;
 	}
-	spin_unlock(&cifs_tcp_ses_lock);
-
 	/* ses_count can never go negative */
 	WARN_ON(ses->ses_count < 0);
 
-	if (ses->ses_status == SES_GOOD)
-		ses->ses_status = SES_EXITING;
-
-	cifs_free_ipc(ses);
+	spin_lock(&ses->chan_lock);
+	cifs_chan_clear_need_reconnect(ses, server);
+	spin_unlock(&ses->chan_lock);
+
+	do_logoff = ses->ses_status == SES_GOOD && server->ops->logoff;
+	ses->ses_status = SES_EXITING;
+	tcon = ses->tcon_ipc;
+	ses->tcon_ipc = NULL;
+ 	spin_unlock(&ses->ses_lock);
+	spin_unlock(&cifs_tcp_ses_lock);
 
-	if (ses->ses_status == SES_EXITING && server->ops->logoff) {
+	/*
+	 * On session close, the IPC is closed and the server must release all
+	 * tcons of the session.  No need to send a tree disconnect here.
+	 *
+	 * Besides, it will make the server to not close durable and resilient
+	 * files on session close, as specified in MS-SMB2 3.3.5.6 Receiving an
+	 * SMB2 LOGOFF Request.
+	 */
+	tconInfoFree(tcon);
+	if (do_logoff) {
 		xid = get_xid();
 		rc = server->ops->logoff(xid, ses);
 		if (rc)
-- 
2.45.2

Re: backporting 24a9799aa8ef ("smb: client: fix UAF in smb2_reconnect_server()") to older stable series

[Greg KH]

On Thu, Dec 12, 2024 at 10:48:55PM +0100, Michael Krause wrote:
> On 12/12/24 1:26 PM, Greg KH wrote:
> > On Tue, Dec 10, 2024 at 12:05:00AM +0100, Michael Krause wrote:
> > > On 12/3/24 3:45 PM, Salvatore Bonaccorso wrote:
> > > > Paulo,
> > > > 
> > > > On Tue, Dec 03, 2024 at 10:18:25AM -0300, Paulo Alcantara wrote:
> > > > > Michael Krause <mk-debian@galax.is> writes:
> > > > > 
> > > > > > On 11/30/24 10:21 AM, Salvatore Bonaccorso wrote:
> > > > > > > Michael, did a manual backport of 24a9799aa8ef ("smb: client: fix UAF
> > > > > > > in smb2_reconnect_server()") which seems in fact to solve the issue.
> > > > > > > 
> > > > > > > Michael, can you please post your backport here for review from Paulo
> > > > > > > and Steve?
> > > > > > 
> > > > > > Of course, attached.
> > > > > > 
> > > > > > Now I really hope I didn't screw it up :)
> > > > > 
> > > > > LGTM.  Thanks Michael for the backport.
> > > > 
> > > > Thanks a lot for the review. So to get it accepted it needs to be
> > > > brough into the form which Greg can pick up. Michael can you do that
> > > > and add your Signed-off line accordingly?
> > > Happy to. Hope this is in the proper format:
> > 
> > It's corrupted somehow:
> > 
> > patching file fs/smb/client/connect.c
> > patch: **** malformed patch at line 202:  		if (rc)
> > 
> > 
> > Can you resend it or attach it?
> > 
> > thanks,
> > 
> > greg k-h
> 
> Ugh, how embarrassing. I'm sorry, I "fixed" some minor whitespace issue directly in the patch and apparently did something wrong.
> 
> I redid the white space fix before diffing again and attach and inline the new version. The chunks are a bit alternated to the earlier version now unfortunately. This one applies..

Doesn't apply for me:

checking file fs/smb/client/connect.c
Hunk #1 FAILED at 259.
Hunk #2 FAILED at 1977.
Hunk #3 FAILED at 2035.
3 out of 3 hunks FAILED
checking file fs/smb/client/connect.c

Any ideas?

thanks,

greg k-h

Re: backporting 24a9799aa8ef ("smb: client: fix UAF in smb2_reconnect_server()") to older stable series

[Salvatore Bonaccorso]

Hi Greg,

On Fri, Dec 13, 2024 at 03:33:31PM +0100, Greg KH wrote:
> On Thu, Dec 12, 2024 at 10:48:55PM +0100, Michael Krause wrote:
> > On 12/12/24 1:26 PM, Greg KH wrote:
> > > On Tue, Dec 10, 2024 at 12:05:00AM +0100, Michael Krause wrote:
> > > > On 12/3/24 3:45 PM, Salvatore Bonaccorso wrote:
> > > > > Paulo,
> > > > > 
> > > > > On Tue, Dec 03, 2024 at 10:18:25AM -0300, Paulo Alcantara wrote:
> > > > > > Michael Krause <mk-debian@galax.is> writes:
> > > > > > 
> > > > > > > On 11/30/24 10:21 AM, Salvatore Bonaccorso wrote:
> > > > > > > > Michael, did a manual backport of 24a9799aa8ef ("smb: client: fix UAF
> > > > > > > > in smb2_reconnect_server()") which seems in fact to solve the issue.
> > > > > > > > 
> > > > > > > > Michael, can you please post your backport here for review from Paulo
> > > > > > > > and Steve?
> > > > > > > 
> > > > > > > Of course, attached.
> > > > > > > 
> > > > > > > Now I really hope I didn't screw it up :)
> > > > > > 
> > > > > > LGTM.  Thanks Michael for the backport.
> > > > > 
> > > > > Thanks a lot for the review. So to get it accepted it needs to be
> > > > > brough into the form which Greg can pick up. Michael can you do that
> > > > > and add your Signed-off line accordingly?
> > > > Happy to. Hope this is in the proper format:
> > > 
> > > It's corrupted somehow:
> > > 
> > > patching file fs/smb/client/connect.c
> > > patch: **** malformed patch at line 202:  		if (rc)
> > > 
> > > 
> > > Can you resend it or attach it?
> > > 
> > > thanks,
> > > 
> > > greg k-h
> > 
> > Ugh, how embarrassing. I'm sorry, I "fixed" some minor whitespace issue directly in the patch and apparently did something wrong.
> > 
> > I redid the white space fix before diffing again and attach and inline the new version. The chunks are a bit alternated to the earlier version now unfortunately. This one applies..
> 
> Doesn't apply for me:
> 
> checking file fs/smb/client/connect.c
> Hunk #1 FAILED at 259.
> Hunk #2 FAILED at 1977.
> Hunk #3 FAILED at 2035.
> 3 out of 3 hunks FAILED
> checking file fs/smb/client/connect.c
> 
> Any ideas?

Hmm, that is strange. I just did the follwoing:

$ git branch 6.1.y-backport-smb-uaf-smb2_reconnect_server v6.1.119
$ git checkout 6.1.y-backport-smb-uaf-smb2_reconnect_server
$ git am /tmp/backport-6.1-smb-client-fix-UAF-in-smb2_reconnect_server.v2.patch
Applying: smb: client: fix UAF in smb2_reconnect_server()
.git/rebase-apply/patch:102: space before tab in indent.
        spin_unlock(&ses->ses_lock);
warning: 1 line adds whitespace errors.

The warning looks correct, there is a space before the indent here:

[...]
180 +^Ido_logoff = ses->ses_status == SES_GOOD && server->ops->logoff;$
181 +^Ises->ses_status = SES_EXITING;$
182 +^Itcon = ses->tcon_ipc;$
183 +^Ises->tcon_ipc = NULL;$
184 + ^Ispin_unlock(&ses->ses_lock);$  <--- space before the indent
tab
185 +^Ispin_unlock(&cifs_tcp_ses_lock);$
186  $
187 -^Iif (ses->ses_status == SES_EXITING && server->ops->logoff) {$
[...]

Regards,
Salvatore

Re: backporting 24a9799aa8ef ("smb: client: fix UAF in smb2_reconnect_server()") to older stable series

[Greg KH]

On Fri, Dec 13, 2024 at 04:53:35PM +0100, Salvatore Bonaccorso wrote:
> Hi Greg,
> 
> On Fri, Dec 13, 2024 at 03:33:31PM +0100, Greg KH wrote:
> > On Thu, Dec 12, 2024 at 10:48:55PM +0100, Michael Krause wrote:
> > > On 12/12/24 1:26 PM, Greg KH wrote:
> > > > On Tue, Dec 10, 2024 at 12:05:00AM +0100, Michael Krause wrote:
> > > > > On 12/3/24 3:45 PM, Salvatore Bonaccorso wrote:
> > > > > > Paulo,
> > > > > > 
> > > > > > On Tue, Dec 03, 2024 at 10:18:25AM -0300, Paulo Alcantara wrote:
> > > > > > > Michael Krause <mk-debian@galax.is> writes:
> > > > > > > 
> > > > > > > > On 11/30/24 10:21 AM, Salvatore Bonaccorso wrote:
> > > > > > > > > Michael, did a manual backport of 24a9799aa8ef ("smb: client: fix UAF
> > > > > > > > > in smb2_reconnect_server()") which seems in fact to solve the issue.
> > > > > > > > > 
> > > > > > > > > Michael, can you please post your backport here for review from Paulo
> > > > > > > > > and Steve?
> > > > > > > > 
> > > > > > > > Of course, attached.
> > > > > > > > 
> > > > > > > > Now I really hope I didn't screw it up :)
> > > > > > > 
> > > > > > > LGTM.  Thanks Michael for the backport.
> > > > > > 
> > > > > > Thanks a lot for the review. So to get it accepted it needs to be
> > > > > > brough into the form which Greg can pick up. Michael can you do that
> > > > > > and add your Signed-off line accordingly?
> > > > > Happy to. Hope this is in the proper format:
> > > > 
> > > > It's corrupted somehow:
> > > > 
> > > > patching file fs/smb/client/connect.c
> > > > patch: **** malformed patch at line 202:  		if (rc)
> > > > 
> > > > 
> > > > Can you resend it or attach it?
> > > > 
> > > > thanks,
> > > > 
> > > > greg k-h
> > > 
> > > Ugh, how embarrassing. I'm sorry, I "fixed" some minor whitespace issue directly in the patch and apparently did something wrong.
> > > 
> > > I redid the white space fix before diffing again and attach and inline the new version. The chunks are a bit alternated to the earlier version now unfortunately. This one applies..
> > 
> > Doesn't apply for me:
> > 
> > checking file fs/smb/client/connect.c
> > Hunk #1 FAILED at 259.
> > Hunk #2 FAILED at 1977.
> > Hunk #3 FAILED at 2035.
> > 3 out of 3 hunks FAILED
> > checking file fs/smb/client/connect.c
> > 
> > Any ideas?
> 
> Hmm, that is strange. I just did the follwoing:
> 
> $ git branch 6.1.y-backport-smb-uaf-smb2_reconnect_server v6.1.119
> $ git checkout 6.1.y-backport-smb-uaf-smb2_reconnect_server
> $ git am /tmp/backport-6.1-smb-client-fix-UAF-in-smb2_reconnect_server.v2.patch
> Applying: smb: client: fix UAF in smb2_reconnect_server()
> .git/rebase-apply/patch:102: space before tab in indent.
>         spin_unlock(&ses->ses_lock);
> warning: 1 line adds whitespace errors.
> 
> The warning looks correct, there is a space before the indent here:
> 
> [...]
> 180 +^Ido_logoff = ses->ses_status == SES_GOOD && server->ops->logoff;$
> 181 +^Ises->ses_status = SES_EXITING;$
> 182 +^Itcon = ses->tcon_ipc;$
> 183 +^Ises->tcon_ipc = NULL;$
> 184 + ^Ispin_unlock(&ses->ses_lock);$  <--- space before the indent
> tab
> 185 +^Ispin_unlock(&cifs_tcp_ses_lock);$
> 186  $
> 187 -^Iif (ses->ses_status == SES_EXITING && server->ops->logoff) {$
> [...]

Ok, this looks like it was a base64 issue on my side, with my tools,
sorry about that.  Now queued up!

greg k-h