Skip to content

Commit 4b3747a

Browse files
pcacjrjaywang-amazon
authored andcommitted
smb: client: fix UAF in smb2_reconnect_server()
commit 24a9799 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> [v5.10: added ses_lock to cifs_ses; used ses->status and old statusEnum values; resolved a few contextual conflicts including clearing bool need_reconnect in cifs_put_smb_ses and moved changes from cifs_mark_tcp_ses_conns_for_reconnect to cifs_reconnect] Signed-off-by: Shaoying Xu <shaoyi@amazon.com>
1 parent 9599b97 commit 4b3747a

File tree

2 files changed

+33
-40
lines changed

2 files changed

+33
-40
lines changed

fs/cifs/cifsglob.h

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -979,6 +979,7 @@ struct cifs_ses {
979979
struct list_head smb_ses_list;
980980
struct list_head tcon_list;
981981
struct cifs_tcon *tcon_ipc;
982+
spinlock_t ses_lock; /* protect anything here that is not protected */
982983
struct mutex session_mutex;
983984
struct TCP_Server_Info *server; /* pointer to server info */
984985
int ses_count; /* reference counter */

fs/cifs/connect.c

Lines changed: 32 additions & 40 deletions
Original file line numberDiff line numberDiff line change
@@ -455,6 +455,13 @@ cifs_reconnect(struct TCP_Server_Info *server)
455455
spin_lock(&cifs_tcp_ses_lock);
456456
list_for_each(tmp, &server->smb_ses_list) {
457457
ses = list_entry(tmp, struct cifs_ses, smb_ses_list);
458+
spin_lock(&ses->ses_lock);
459+
if (ses->status == CifsExiting) {
460+
spin_unlock(&ses->ses_lock);
461+
continue;
462+
}
463+
464+
spin_unlock(&ses->ses_lock);
458465
ses->need_reconnect = true;
459466
list_for_each(tmp2, &ses->tcon_list) {
460467
tcon = list_entry(tmp2, struct cifs_tcon, tcon_list);
@@ -2781,34 +2788,6 @@ cifs_setup_ipc(struct cifs_ses *ses, struct smb_vol *volume_info)
27812788
return rc;
27822789
}
27832790

2784-
/**
2785-
* cifs_free_ipc - helper to release the session IPC tcon
2786-
*
2787-
* Needs to be called everytime a session is destroyed
2788-
*/
2789-
static int
2790-
cifs_free_ipc(struct cifs_ses *ses)
2791-
{
2792-
int rc = 0, xid;
2793-
struct cifs_tcon *tcon = ses->tcon_ipc;
2794-
2795-
if (tcon == NULL)
2796-
return 0;
2797-
2798-
if (ses->server->ops->tree_disconnect) {
2799-
xid = get_xid();
2800-
rc = ses->server->ops->tree_disconnect(xid, tcon);
2801-
free_xid(xid);
2802-
}
2803-
2804-
if (rc)
2805-
cifs_dbg(FYI, "failed to disconnect IPC tcon (rc=%d)\n", rc);
2806-
2807-
tconInfoFree(tcon);
2808-
ses->tcon_ipc = NULL;
2809-
return rc;
2810-
}
2811-
28122791
static struct cifs_ses *
28132792
cifs_find_smb_ses(struct TCP_Server_Info *server, struct smb_vol *vol)
28142793
{
@@ -2833,28 +2812,41 @@ void cifs_put_smb_ses(struct cifs_ses *ses)
28332812
unsigned int rc, xid;
28342813
unsigned int chan_count;
28352814
struct TCP_Server_Info *server = ses->server;
2815+
struct cifs_tcon *tcon;
2816+
bool do_logoff;
28362817

28372818
cifs_dbg(FYI, "%s: ses_count=%d\n", __func__, ses->ses_count);
28382819

28392820
spin_lock(&cifs_tcp_ses_lock);
2840-
if (ses->status == CifsExiting) {
2841-
spin_unlock(&cifs_tcp_ses_lock);
2842-
return;
2843-
}
2844-
if (--ses->ses_count > 0) {
2821+
spin_lock(&ses->ses_lock);
2822+
cifs_dbg(FYI, "%s: id=0x%llx ses_count=%d ses_status=%u ipc=%s\n",
2823+
__func__, ses->Suid, ses->ses_count, ses->status,
2824+
ses->tcon_ipc ? ses->tcon_ipc->treeName : "none");
2825+
if (ses->status == CifsExiting || --ses->ses_count > 0) {
2826+
spin_unlock(&ses->ses_lock);
28452827
spin_unlock(&cifs_tcp_ses_lock);
28462828
return;
28472829
}
2848-
spin_unlock(&cifs_tcp_ses_lock);
28492830

2850-
spin_lock(&GlobalMid_Lock);
2851-
if (ses->status == CifsGood)
2852-
ses->status = CifsExiting;
2853-
spin_unlock(&GlobalMid_Lock);
2831+
ses->need_reconnect = false;
28542832

2855-
cifs_free_ipc(ses);
2833+
do_logoff = ses->status == CifsGood && server->ops->logoff;
2834+
ses->status = CifsExiting;
2835+
tcon = ses->tcon_ipc;
2836+
ses->tcon_ipc = NULL;
2837+
spin_unlock(&ses->ses_lock);
2838+
spin_unlock(&cifs_tcp_ses_lock);
28562839

2857-
if (ses->status == CifsExiting && server->ops->logoff) {
2840+
/*
2841+
* On session close, the IPC is closed and the server must release all
2842+
* tcons of the session. No need to send a tree disconnect here.
2843+
*
2844+
* Besides, it will make the server to not close durable and resilient
2845+
* files on session close, as specified in MS-SMB2 3.3.5.6 Receiving an
2846+
* SMB2 LOGOFF Request.
2847+
*/
2848+
tconInfoFree(tcon);
2849+
if (do_logoff) {
28582850
xid = get_xid();
28592851
rc = server->ops->logoff(xid, ses);
28602852
if (rc)

0 commit comments

Comments
 (0)