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

[Salvatore Bonaccorso <carnil@debian.org>]

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