[PATCH 1/4] smb: client: remove extra @chan_count check in __cifs_put_smb_ses()

[PATCH 1/4] smb: client: remove extra @chan_count check in __cifs_put_smb_ses()

[Paulo Alcantara]

If @ses->chan_count <= 1, then for-loop body will not be executed so
no need to check it twice.

Signed-off-by: Paulo Alcantara (SUSE) <pc@manguebit.com>
---
 fs/smb/client/connect.c | 23 +++++++++--------------
 1 file changed, 9 insertions(+), 14 deletions(-)

diff --git a/fs/smb/client/connect.c b/fs/smb/client/connect.c
index 7b923e36501b..a017ee552256 100644
--- a/fs/smb/client/connect.c
+++ b/fs/smb/client/connect.c
@@ -1969,9 +1969,10 @@ cifs_find_smb_ses(struct TCP_Server_Info *server, struct smb3_fs_context *ctx)
 
 void __cifs_put_smb_ses(struct cifs_ses *ses)
 {
-	unsigned int rc, xid;
-	unsigned int chan_count;
 	struct TCP_Server_Info *server = ses->server;
+	unsigned int xid;
+	size_t i;
+	int rc;
 
 	spin_lock(&ses->ses_lock);
 	if (ses->ses_status == SES_EXITING) {
@@ -2017,20 +2018,14 @@ void __cifs_put_smb_ses(struct cifs_ses *ses)
 	list_del_init(&ses->smb_ses_list);
 	spin_unlock(&cifs_tcp_ses_lock);
 
-	chan_count = ses->chan_count;
-
 	/* close any extra channels */
-	if (chan_count > 1) {
-		int i;
-
-		for (i = 1; i < chan_count; i++) {
-			if (ses->chans[i].iface) {
-				kref_put(&ses->chans[i].iface->refcount, release_iface);
-				ses->chans[i].iface = NULL;
-			}
-			cifs_put_tcp_session(ses->chans[i].server, 0);
-			ses->chans[i].server = NULL;
+	for (i = 1; i < ses->chan_count; i++) {
+		if (ses->chans[i].iface) {
+			kref_put(&ses->chans[i].iface->refcount, release_iface);
+			ses->chans[i].iface = NULL;
 		}
+		cifs_put_tcp_session(ses->chans[i].server, 0);
+		ses->chans[i].server = NULL;
 	}
 
 	sesInfoFree(ses);
-- 
2.42.0

[PATCH 2/4] smb: client: fix use-after-free bug in cifs_debug_data_proc_show()

[Paulo Alcantara]

Skip SMB sessions that are being teared down
(e.g. @ses->ses_status == SES_EXITING) in cifs_debug_data_proc_show()
to avoid use-after-free in @ses.

This fixes the following GPF when reading from /proc/fs/cifs/DebugData
while mounting and umounting

  [ 816.251274] general protection fault, probably for non-canonical
  address 0x6b6b6b6b6b6b6d81: 0000 [#1] PREEMPT SMP NOPTI
  ...
  [  816.260138] Call Trace:
  [  816.260329]  <TASK>
  [  816.260499]  ? die_addr+0x36/0x90
  [  816.260762]  ? exc_general_protection+0x1b3/0x410
  [  816.261126]  ? asm_exc_general_protection+0x26/0x30
  [  816.261502]  ? cifs_debug_tcon+0xbd/0x240 [cifs]
  [  816.261878]  ? cifs_debug_tcon+0xab/0x240 [cifs]
  [  816.262249]  cifs_debug_data_proc_show+0x516/0xdb0 [cifs]
  [  816.262689]  ? seq_read_iter+0x379/0x470
  [  816.262995]  seq_read_iter+0x118/0x470
  [  816.263291]  proc_reg_read_iter+0x53/0x90
  [  816.263596]  ? srso_alias_return_thunk+0x5/0x7f
  [  816.263945]  vfs_read+0x201/0x350
  [  816.264211]  ksys_read+0x75/0x100
  [  816.264472]  do_syscall_64+0x3f/0x90
  [  816.264750]  entry_SYSCALL_64_after_hwframe+0x6e/0xd8
  [  816.265135] RIP: 0033:0x7fd5e669d381

Cc: Frank Sorenson <sorenson@redhat.com>
Signed-off-by: Paulo Alcantara (SUSE) <pc@manguebit.com>
---
 fs/smb/client/cifs_debug.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/fs/smb/client/cifs_debug.c b/fs/smb/client/cifs_debug.c
index 76922fcc4bc6..9a0ccd87468e 100644
--- a/fs/smb/client/cifs_debug.c
+++ b/fs/smb/client/cifs_debug.c
@@ -452,6 +452,11 @@ static int cifs_debug_data_proc_show(struct seq_file *m, void *v)
 		seq_printf(m, "\n\n\tSessions: ");
 		i = 0;
 		list_for_each_entry(ses, &server->smb_ses_list, smb_ses_list) {
+			spin_lock(&ses->ses_lock);
+			if (ses->ses_status == SES_EXITING) {
+				spin_unlock(&ses->ses_lock);
+				continue;
+			}
 			i++;
 			if ((ses->serverDomain == NULL) ||
 				(ses->serverOS == NULL) ||
@@ -472,6 +477,7 @@ static int cifs_debug_data_proc_show(struct seq_file *m, void *v)
 				ses->ses_count, ses->serverOS, ses->serverNOS,
 				ses->capabilities, ses->ses_status);
 			}
+			spin_unlock(&ses->ses_lock);
 
 			seq_printf(m, "\n\tSecurity type: %s ",
 				get_security_type_str(server->ops->select_sectype(server, ses->sectype)));
-- 
2.42.0

Re: [PATCH 2/4] smb: client: fix use-after-free bug in cifs_debug_data_proc_show()

[Steve French]

fix was already in cifs-2.6.git for-next

Added Cc: stable to it though

Let me know if you see something missing (or if patch changed from the
previous one eg)

On Mon, Oct 30, 2023 at 3:20 PM Paulo Alcantara <pc@manguebit.com> wrote:
>
> Skip SMB sessions that are being teared down
> (e.g. @ses->ses_status == SES_EXITING) in cifs_debug_data_proc_show()
> to avoid use-after-free in @ses.
>
> This fixes the following GPF when reading from /proc/fs/cifs/DebugData
> while mounting and umounting
>
>   [ 816.251274] general protection fault, probably for non-canonical
>   address 0x6b6b6b6b6b6b6d81: 0000 [#1] PREEMPT SMP NOPTI
>   ...
>   [  816.260138] Call Trace:
>   [  816.260329]  <TASK>
>   [  816.260499]  ? die_addr+0x36/0x90
>   [  816.260762]  ? exc_general_protection+0x1b3/0x410
>   [  816.261126]  ? asm_exc_general_protection+0x26/0x30
>   [  816.261502]  ? cifs_debug_tcon+0xbd/0x240 [cifs]
>   [  816.261878]  ? cifs_debug_tcon+0xab/0x240 [cifs]
>   [  816.262249]  cifs_debug_data_proc_show+0x516/0xdb0 [cifs]
>   [  816.262689]  ? seq_read_iter+0x379/0x470
>   [  816.262995]  seq_read_iter+0x118/0x470
>   [  816.263291]  proc_reg_read_iter+0x53/0x90
>   [  816.263596]  ? srso_alias_return_thunk+0x5/0x7f
>   [  816.263945]  vfs_read+0x201/0x350
>   [  816.264211]  ksys_read+0x75/0x100
>   [  816.264472]  do_syscall_64+0x3f/0x90
>   [  816.264750]  entry_SYSCALL_64_after_hwframe+0x6e/0xd8
>   [  816.265135] RIP: 0033:0x7fd5e669d381
>
> Cc: Frank Sorenson <sorenson@redhat.com>
> Signed-off-by: Paulo Alcantara (SUSE) <pc@manguebit.com>
> ---
>  fs/smb/client/cifs_debug.c | 6 ++++++
>  1 file changed, 6 insertions(+)
>
> diff --git a/fs/smb/client/cifs_debug.c b/fs/smb/client/cifs_debug.c
> index 76922fcc4bc6..9a0ccd87468e 100644
> --- a/fs/smb/client/cifs_debug.c
> +++ b/fs/smb/client/cifs_debug.c
> @@ -452,6 +452,11 @@ static int cifs_debug_data_proc_show(struct seq_file *m, void *v)
>                 seq_printf(m, "\n\n\tSessions: ");
>                 i = 0;
>                 list_for_each_entry(ses, &server->smb_ses_list, smb_ses_list) {
> +                       spin_lock(&ses->ses_lock);
> +                       if (ses->ses_status == SES_EXITING) {
> +                               spin_unlock(&ses->ses_lock);
> +                               continue;
> +                       }
>                         i++;
>                         if ((ses->serverDomain == NULL) ||
>                                 (ses->serverOS == NULL) ||
> @@ -472,6 +477,7 @@ static int cifs_debug_data_proc_show(struct seq_file *m, void *v)
>                                 ses->ses_count, ses->serverOS, ses->serverNOS,
>                                 ses->capabilities, ses->ses_status);
>                         }
> +                       spin_unlock(&ses->ses_lock);
>
>                         seq_printf(m, "\n\tSecurity type: %s ",
>                                 get_security_type_str(server->ops->select_sectype(server, ses->sectype)));
> --
> 2.42.0
>


-- 
Thanks,

Steve

Re: [PATCH 2/4] smb: client: fix use-after-free bug in cifs_debug_data_proc_show()

[Wang Zhaolong]

Hello,

I encountered some confusion while reviewing the source code related to
CVE-2023-52752.

I was able to reproduce the issue, and the original problem seems to be:

---
process 1                   process 2(read /proc/fs/cifs/DebugData)

cifs_umount
cifs_put_tlink
cifs_put_tcon
cifs_put_smb_ses                cifs_debug_data_proc_show
   spin_unlock(&cifs_tcp_ses_lock)
                                   spin_lock(&cifs_tcp_ses_lock);
                                   list_for_each...(ses,server->smb_ses_list,...)
   cifs_free_ipc
     tconInfoFree(tcon)
                                   if (ses->tcon_ipc)
                                    cifs_debug_tcon(m,ses->tcon_ipc)
                                      // UAF
     ses->tcon_ipc = NULLl
                                   spin_unlock(&cifs_tcp_ses_lock);

   spin_lock(&cifs_tcp_ses_lock)
   list_del_init(&ses->smb_ses_list)
   spin_unlock(&cifs_tcp_ses_lock)
---

In commit ff7d80a9f271 ("cifs: fix session state transition to avoid use-after-free
issue"), setting ses_status to SES_EXITING was moved under the protection of
cifs_tcp_ses_lock.

In cifs_debug_data_proc_show(), the logic that checks ses->ses_status == SES_EXITING
already seems sufficient to avoid this issue. Therefore, it appears that ses->ses_lock
might not be necessary. Additionally, I am curious why ses->ses_lock needs to cover
such a large scope.


> diff --git a/fs/smb/client/cifs_debug.c b/fs/smb/client/cifs_debug.c
> index 76922fcc4bc6..9a0ccd87468e 100644
> --- a/fs/smb/client/cifs_debug.c
> +++ b/fs/smb/client/cifs_debug.c
> @@ -452,6 +452,11 @@ static int cifs_debug_data_proc_show(struct seq_file *m, void *v)
>   		seq_printf(m, "\n\n\tSessions: ");
>   		i = 0;
>   		list_for_each_entry(ses, &server->smb_ses_list, smb_ses_list) {
> +			spin_lock(&ses->ses_lock);
> +			if (ses->ses_status == SES_EXITING) {
> +				spin_unlock(&ses->ses_lock);
> +				continue;
> +			}
>   			i++;
>   			if ((ses->serverDomain == NULL) ||
>   				(ses->serverOS == NULL) ||
> @@ -472,6 +477,7 @@ static int cifs_debug_data_proc_show(struct seq_file *m, void *v)
>   				ses->ses_count, ses->serverOS, ses->serverNOS,
>   				ses->capabilities, ses->ses_status);
>   			}
> +			spin_unlock(&ses->ses_lock);
>   
>   			seq_printf(m, "\n\tSecurity type: %s ",
>   				get_security_type_str(server->ops->select_sectype(server, ses->sectype)));

I believe in the latest mainline, this could potentially be modified to:

```
diff --git a/fs/smb/client/cifs_debug.c b/fs/smb/client/cifs_debug.c
index c71ae5c04306..2d9e83b71643 100644
--- a/fs/smb/client/cifs_debug.c
+++ b/fs/smb/client/cifs_debug.c
@@ -485,11 +485,8 @@ static int cifs_debug_data_proc_show(struct seq_file *m, void *v)
  		seq_printf(m, "\n\n\tSessions: ");
  		i = 0;
  		list_for_each_entry(ses, &server->smb_ses_list, smb_ses_list) {
-			spin_lock(&ses->ses_lock);
-			if (ses->ses_status == SES_EXITING) {
-				spin_unlock(&ses->ses_lock);
+			if (cifs_ses_exiting(ses))
  				continue;
-			}
  			i++;
  			if ((ses->serverDomain == NULL) ||
  				(ses->serverOS == NULL) ||
@@ -512,7 +509,6 @@ static int cifs_debug_data_proc_show(struct seq_file *m, void *v)
  			}
  			if (ses->expired_pwd)
  				seq_puts(m, "password no longer valid ");
-			spin_unlock(&ses->ses_lock);
  
  			seq_printf(m, "\n\tSecurity type: %s ",
  				get_security_type_str(server->ops->select_sectype(server, ses->sectype)));

```

Best regards,
Wang Zhaolong

[PATCH 3/4] smb: client: fix potential deadlock when releasing mids

[Paulo Alcantara]

All release_mid() callers seem to hold a reference of @mid so there is
no need to call kref_put(&mid->refcount, __release_mid) under
@server->mid_lock spinlock.  If they don't, then an use-after-free bug
would have occurred anyway.

By getting rid of such spinlock also fixes a potential deadlock as
shown below

CPU 0                                CPU 1
------------------------------------------------------------------
cifs_demultiplex_thread()            cifs_debug_data_proc_show()
 release_mid()
  spin_lock(&server->mid_lock);
                                     spin_lock(&cifs_tcp_ses_lock)
				      spin_lock(&server->mid_lock)
  __release_mid()
   smb2_find_smb_tcon()
    spin_lock(&cifs_tcp_ses_lock) *deadlock*

Cc: Frank Sorenson <sorenson@redhat.com>
Signed-off-by: Paulo Alcantara (SUSE) <pc@manguebit.com>
---
 fs/smb/client/cifsproto.h |  7 ++++++-
 fs/smb/client/smb2misc.c  |  2 +-
 fs/smb/client/transport.c | 11 +----------
 3 files changed, 8 insertions(+), 12 deletions(-)

diff --git a/fs/smb/client/cifsproto.h b/fs/smb/client/cifsproto.h
index 0c37eefa18a5..890ceddae07e 100644
--- a/fs/smb/client/cifsproto.h
+++ b/fs/smb/client/cifsproto.h
@@ -81,7 +81,7 @@ extern char *cifs_build_path_to_root(struct smb3_fs_context *ctx,
 extern char *build_wildcard_path_from_dentry(struct dentry *direntry);
 char *cifs_build_devname(char *nodename, const char *prepath);
 extern void delete_mid(struct mid_q_entry *mid);
-extern void release_mid(struct mid_q_entry *mid);
+void __release_mid(struct kref *refcount);
 extern void cifs_wake_up_task(struct mid_q_entry *mid);
 extern int cifs_handle_standard(struct TCP_Server_Info *server,
 				struct mid_q_entry *mid);
@@ -740,4 +740,9 @@ static inline bool dfs_src_pathname_equal(const char *s1, const char *s2)
 	return true;
 }
 
+static inline void release_mid(struct mid_q_entry *mid)
+{
+	kref_put(&mid->refcount, __release_mid);
+}
+
 #endif			/* _CIFSPROTO_H */
diff --git a/fs/smb/client/smb2misc.c b/fs/smb/client/smb2misc.c
index 25f7cd6f23d6..32dfa0f7a78c 100644
--- a/fs/smb/client/smb2misc.c
+++ b/fs/smb/client/smb2misc.c
@@ -787,7 +787,7 @@ __smb2_handle_cancelled_cmd(struct cifs_tcon *tcon, __u16 cmd, __u64 mid,
 {
 	struct close_cancelled_open *cancelled;
 
-	cancelled = kzalloc(sizeof(*cancelled), GFP_ATOMIC);
+	cancelled = kzalloc(sizeof(*cancelled), GFP_KERNEL);
 	if (!cancelled)
 		return -ENOMEM;
 
diff --git a/fs/smb/client/transport.c b/fs/smb/client/transport.c
index 14710afdc2a3..d553b7a54621 100644
--- a/fs/smb/client/transport.c
+++ b/fs/smb/client/transport.c
@@ -76,7 +76,7 @@ alloc_mid(const struct smb_hdr *smb_buffer, struct TCP_Server_Info *server)
 	return temp;
 }
 
-static void __release_mid(struct kref *refcount)
+void __release_mid(struct kref *refcount)
 {
 	struct mid_q_entry *midEntry =
 			container_of(refcount, struct mid_q_entry, refcount);
@@ -156,15 +156,6 @@ static void __release_mid(struct kref *refcount)
 	mempool_free(midEntry, cifs_mid_poolp);
 }
 
-void release_mid(struct mid_q_entry *mid)
-{
-	struct TCP_Server_Info *server = mid->server;
-
-	spin_lock(&server->mid_lock);
-	kref_put(&mid->refcount, __release_mid);
-	spin_unlock(&server->mid_lock);
-}
-
 void
 delete_mid(struct mid_q_entry *mid)
 {
-- 
2.42.0

Re: [PATCH 3/4] smb: client: fix potential deadlock when releasing mids

[Steve French]

fix was already in cifs-2.6.git for-next, but added Cc: stable

Let me know if I missed anything

On Mon, Oct 30, 2023 at 3:20 PM Paulo Alcantara <pc@manguebit.com> wrote:
>
> All release_mid() callers seem to hold a reference of @mid so there is
> no need to call kref_put(&mid->refcount, __release_mid) under
> @server->mid_lock spinlock.  If they don't, then an use-after-free bug
> would have occurred anyway.
>
> By getting rid of such spinlock also fixes a potential deadlock as
> shown below
>
> CPU 0                                CPU 1
> ------------------------------------------------------------------
> cifs_demultiplex_thread()            cifs_debug_data_proc_show()
>  release_mid()
>   spin_lock(&server->mid_lock);
>                                      spin_lock(&cifs_tcp_ses_lock)
>                                       spin_lock(&server->mid_lock)
>   __release_mid()
>    smb2_find_smb_tcon()
>     spin_lock(&cifs_tcp_ses_lock) *deadlock*
>
> Cc: Frank Sorenson <sorenson@redhat.com>
> Signed-off-by: Paulo Alcantara (SUSE) <pc@manguebit.com>
> ---
>  fs/smb/client/cifsproto.h |  7 ++++++-
>  fs/smb/client/smb2misc.c  |  2 +-
>  fs/smb/client/transport.c | 11 +----------
>  3 files changed, 8 insertions(+), 12 deletions(-)
>
> diff --git a/fs/smb/client/cifsproto.h b/fs/smb/client/cifsproto.h
> index 0c37eefa18a5..890ceddae07e 100644
> --- a/fs/smb/client/cifsproto.h
> +++ b/fs/smb/client/cifsproto.h
> @@ -81,7 +81,7 @@ extern char *cifs_build_path_to_root(struct smb3_fs_context *ctx,
>  extern char *build_wildcard_path_from_dentry(struct dentry *direntry);
>  char *cifs_build_devname(char *nodename, const char *prepath);
>  extern void delete_mid(struct mid_q_entry *mid);
> -extern void release_mid(struct mid_q_entry *mid);
> +void __release_mid(struct kref *refcount);
>  extern void cifs_wake_up_task(struct mid_q_entry *mid);
>  extern int cifs_handle_standard(struct TCP_Server_Info *server,
>                                 struct mid_q_entry *mid);
> @@ -740,4 +740,9 @@ static inline bool dfs_src_pathname_equal(const char *s1, const char *s2)
>         return true;
>  }
>
> +static inline void release_mid(struct mid_q_entry *mid)
> +{
> +       kref_put(&mid->refcount, __release_mid);
> +}
> +
>  #endif                 /* _CIFSPROTO_H */
> diff --git a/fs/smb/client/smb2misc.c b/fs/smb/client/smb2misc.c
> index 25f7cd6f23d6..32dfa0f7a78c 100644
> --- a/fs/smb/client/smb2misc.c
> +++ b/fs/smb/client/smb2misc.c
> @@ -787,7 +787,7 @@ __smb2_handle_cancelled_cmd(struct cifs_tcon *tcon, __u16 cmd, __u64 mid,
>  {
>         struct close_cancelled_open *cancelled;
>
> -       cancelled = kzalloc(sizeof(*cancelled), GFP_ATOMIC);
> +       cancelled = kzalloc(sizeof(*cancelled), GFP_KERNEL);
>         if (!cancelled)
>                 return -ENOMEM;
>
> diff --git a/fs/smb/client/transport.c b/fs/smb/client/transport.c
> index 14710afdc2a3..d553b7a54621 100644
> --- a/fs/smb/client/transport.c
> +++ b/fs/smb/client/transport.c
> @@ -76,7 +76,7 @@ alloc_mid(const struct smb_hdr *smb_buffer, struct TCP_Server_Info *server)
>         return temp;
>  }
>
> -static void __release_mid(struct kref *refcount)
> +void __release_mid(struct kref *refcount)
>  {
>         struct mid_q_entry *midEntry =
>                         container_of(refcount, struct mid_q_entry, refcount);
> @@ -156,15 +156,6 @@ static void __release_mid(struct kref *refcount)
>         mempool_free(midEntry, cifs_mid_poolp);
>  }
>
> -void release_mid(struct mid_q_entry *mid)
> -{
> -       struct TCP_Server_Info *server = mid->server;
> -
> -       spin_lock(&server->mid_lock);
> -       kref_put(&mid->refcount, __release_mid);
> -       spin_unlock(&server->mid_lock);
> -}
> -
>  void
>  delete_mid(struct mid_q_entry *mid)
>  {
> --
> 2.42.0
>


-- 
Thanks,

Steve

[PATCH 4/4] smb: client: fix use-after-free in smb2_query_info_compound()

[Paulo Alcantara]

The following UAF was triggered when running fstests generic/072 with
KASAN enabled against Windows Server 2022 and mount options
'multichannel,max_channels=2,vers=3.1.1,mfsymlinks,noperm'

  BUG: KASAN: slab-use-after-free in smb2_query_info_compound+0x423/0x6d0 [cifs]
  Read of size 8 at addr ffff888014941048 by task xfs_io/27534

  CPU: 0 PID: 27534 Comm: xfs_io Not tainted 6.6.0-rc7 #1
  Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS
  rel-1.16.2-3-gd478f380-rebuilt.opensuse.org 04/01/2014
  Call Trace:
   dump_stack_lvl+0x4a/0x80
   print_report+0xcf/0x650
   ? srso_alias_return_thunk+0x5/0x7f
   ? srso_alias_return_thunk+0x5/0x7f
   ? __phys_addr+0x46/0x90
   kasan_report+0xda/0x110
   ? smb2_query_info_compound+0x423/0x6d0 [cifs]
   ? smb2_query_info_compound+0x423/0x6d0 [cifs]
   smb2_query_info_compound+0x423/0x6d0 [cifs]
   ? __pfx_smb2_query_info_compound+0x10/0x10 [cifs]
   ? srso_alias_return_thunk+0x5/0x7f
   ? __stack_depot_save+0x39/0x480
   ? kasan_save_stack+0x33/0x60
   ? kasan_set_track+0x25/0x30
   ? ____kasan_slab_free+0x126/0x170
   smb2_queryfs+0xc2/0x2c0 [cifs]
   ? __pfx_smb2_queryfs+0x10/0x10 [cifs]
   ? __pfx___lock_acquire+0x10/0x10
   smb311_queryfs+0x210/0x220 [cifs]
   ? __pfx_smb311_queryfs+0x10/0x10 [cifs]
   ? srso_alias_return_thunk+0x5/0x7f
   ? __lock_acquire+0x480/0x26c0
   ? lock_release+0x1ed/0x640
   ? srso_alias_return_thunk+0x5/0x7f
   ? do_raw_spin_unlock+0x9b/0x100
   cifs_statfs+0x18c/0x4b0 [cifs]
   statfs_by_dentry+0x9b/0xf0
   fd_statfs+0x4e/0xb0
   __do_sys_fstatfs+0x7f/0xe0
   ? __pfx___do_sys_fstatfs+0x10/0x10
   ? srso_alias_return_thunk+0x5/0x7f
   ? lockdep_hardirqs_on_prepare+0x136/0x200
   ? srso_alias_return_thunk+0x5/0x7f
   do_syscall_64+0x3f/0x90
   entry_SYSCALL_64_after_hwframe+0x6e/0xd8

  Allocated by task 27534:
   kasan_save_stack+0x33/0x60
   kasan_set_track+0x25/0x30
   __kasan_kmalloc+0x8f/0xa0
   open_cached_dir+0x71b/0x1240 [cifs]
   smb2_query_info_compound+0x5c3/0x6d0 [cifs]
   smb2_queryfs+0xc2/0x2c0 [cifs]
   smb311_queryfs+0x210/0x220 [cifs]
   cifs_statfs+0x18c/0x4b0 [cifs]
   statfs_by_dentry+0x9b/0xf0
   fd_statfs+0x4e/0xb0
   __do_sys_fstatfs+0x7f/0xe0
   do_syscall_64+0x3f/0x90
   entry_SYSCALL_64_after_hwframe+0x6e/0xd8

  Freed by task 27534:
   kasan_save_stack+0x33/0x60
   kasan_set_track+0x25/0x30
   kasan_save_free_info+0x2b/0x50
   ____kasan_slab_free+0x126/0x170
   slab_free_freelist_hook+0xd0/0x1e0
   __kmem_cache_free+0x9d/0x1b0
   open_cached_dir+0xff5/0x1240 [cifs]
   smb2_query_info_compound+0x5c3/0x6d0 [cifs]
   smb2_queryfs+0xc2/0x2c0 [cifs]

This is a race between open_cached_dir() and cached_dir_lease_break()
where the cache entry for the open directory handle receives a lease
break while creating it.  And before returning from open_cached_dir(),
we put the last reference of the new @cfid because of
!@cfid->has_lease.

Besides the UAF, while running xfstests a lot of missed lease breaks
have been noticed in tests that run several concurrent statfs(2) calls
on those cached fids

  CIFS: VFS: \\w22-root1.gandalf.test No task to wake, unknown frame...
  CIFS: VFS: \\w22-root1.gandalf.test Cmd: 18 Err: 0x0 Flags: 0x1...
  CIFS: VFS: \\w22-root1.gandalf.test smb buf 00000000715bfe83 len 108
  CIFS: VFS: Dump pending requests:
  CIFS: VFS: \\w22-root1.gandalf.test No task to wake, unknown frame...
  CIFS: VFS: \\w22-root1.gandalf.test Cmd: 18 Err: 0x0 Flags: 0x1...
  CIFS: VFS: \\w22-root1.gandalf.test smb buf 000000005aa7316e len 108
  ...

To fix both, in open_cached_dir() ensure that @cfid->has_lease is set
right before sending out compounded request so that any potential
lease break will be get processed by demultiplex thread while we're
still caching @cfid.  And, if open failed for some reason, re-check
@cfid->has_lease to decide whether or not put lease reference.

Signed-off-by: Paulo Alcantara (SUSE) <pc@manguebit.com>
---
 fs/smb/client/cached_dir.c | 84 ++++++++++++++++++++++----------------
 1 file changed, 49 insertions(+), 35 deletions(-)

diff --git a/fs/smb/client/cached_dir.c b/fs/smb/client/cached_dir.c
index fe1bf5b6e0cb..59f6b8e32cc9 100644
--- a/fs/smb/client/cached_dir.c
+++ b/fs/smb/client/cached_dir.c
@@ -32,7 +32,7 @@ static struct cached_fid *find_or_create_cached_dir(struct cached_fids *cfids,
 			 * fully cached or it may be in the process of
 			 * being deleted due to a lease break.
 			 */
-			if (!cfid->has_lease) {
+			if (!cfid->time || !cfid->has_lease) {
 				spin_unlock(&cfids->cfid_list_lock);
 				return NULL;
 			}
@@ -193,10 +193,20 @@ int open_cached_dir(unsigned int xid, struct cifs_tcon *tcon,
 	npath = path_no_prefix(cifs_sb, path);
 	if (IS_ERR(npath)) {
 		rc = PTR_ERR(npath);
-		kfree(utf16_path);
-		return rc;
+		goto out;
 	}
 
+	if (!npath[0]) {
+		dentry = dget(cifs_sb->root);
+	} else {
+		dentry = path_to_dentry(cifs_sb, npath);
+		if (IS_ERR(dentry)) {
+			rc = -ENOENT;
+			goto out;
+		}
+	}
+	cfid->dentry = dentry;
+
 	/*
 	 * We do not hold the lock for the open because in case
 	 * SMB2_open needs to reconnect.
@@ -249,6 +259,15 @@ int open_cached_dir(unsigned int xid, struct cifs_tcon *tcon,
 
 	smb2_set_related(&rqst[1]);
 
+	/*
+	 * Set @cfid->has_lease to true before sending out compounded request so
+	 * its lease reference can be put in cached_dir_lease_break() due to a
+	 * potential lease break right after the request is sent or while @cfid
+	 * is still being cached.  Concurrent processes won't be to use it yet
+	 * due to @cfid->time being zero.
+	 */
+	cfid->has_lease = true;
+
 	rc = compound_send_recv(xid, ses, server,
 				flags, 2, rqst,
 				resp_buftype, rsp_iov);
@@ -263,6 +282,8 @@ int open_cached_dir(unsigned int xid, struct cifs_tcon *tcon,
 	cfid->tcon = tcon;
 	cfid->is_open = true;
 
+	spin_lock(&cfids->cfid_list_lock);
+
 	o_rsp = (struct smb2_create_rsp *)rsp_iov[0].iov_base;
 	oparms.fid->persistent_fid = o_rsp->PersistentFileId;
 	oparms.fid->volatile_fid = o_rsp->VolatileFileId;
@@ -270,18 +291,25 @@ int open_cached_dir(unsigned int xid, struct cifs_tcon *tcon,
 	oparms.fid->mid = le64_to_cpu(o_rsp->hdr.MessageId);
 #endif /* CIFS_DEBUG2 */
 
-	if (o_rsp->OplockLevel != SMB2_OPLOCK_LEVEL_LEASE)
+	rc = -EINVAL;
+	if (o_rsp->OplockLevel != SMB2_OPLOCK_LEVEL_LEASE) {
+		spin_unlock(&cfids->cfid_list_lock);
 		goto oshr_free;
+	}
 
 	smb2_parse_contexts(server, o_rsp,
 			    &oparms.fid->epoch,
 			    oparms.fid->lease_key, &oplock,
 			    NULL, NULL);
-	if (!(oplock & SMB2_LEASE_READ_CACHING_HE))
+	if (!(oplock & SMB2_LEASE_READ_CACHING_HE)) {
+		spin_unlock(&cfids->cfid_list_lock);
 		goto oshr_free;
+	}
 	qi_rsp = (struct smb2_query_info_rsp *)rsp_iov[1].iov_base;
-	if (le32_to_cpu(qi_rsp->OutputBufferLength) < sizeof(struct smb2_file_all_info))
+	if (le32_to_cpu(qi_rsp->OutputBufferLength) < sizeof(struct smb2_file_all_info)) {
+		spin_unlock(&cfids->cfid_list_lock);
 		goto oshr_free;
+	}
 	if (!smb2_validate_and_copy_iov(
 				le16_to_cpu(qi_rsp->OutputBufferOffset),
 				sizeof(struct smb2_file_all_info),
@@ -289,37 +317,24 @@ int open_cached_dir(unsigned int xid, struct cifs_tcon *tcon,
 				(char *)&cfid->file_all_info))
 		cfid->file_all_info_is_valid = true;
 
-	if (!npath[0])
-		dentry = dget(cifs_sb->root);
-	else {
-		dentry = path_to_dentry(cifs_sb, npath);
-		if (IS_ERR(dentry)) {
-			rc = -ENOENT;
-			goto oshr_free;
-		}
-	}
-	spin_lock(&cfids->cfid_list_lock);
-	cfid->dentry = dentry;
 	cfid->time = jiffies;
-	cfid->has_lease = true;
 	spin_unlock(&cfids->cfid_list_lock);
+	/* At this point the directory handle is fully cached */
+	rc = 0;
 
 oshr_free:
-	kfree(utf16_path);
 	SMB2_open_free(&rqst[0]);
 	SMB2_query_info_free(&rqst[1]);
 	free_rsp_buf(resp_buftype[0], rsp_iov[0].iov_base);
 	free_rsp_buf(resp_buftype[1], rsp_iov[1].iov_base);
-	spin_lock(&cfids->cfid_list_lock);
-	if (!cfid->has_lease) {
-		if (rc) {
-			if (cfid->on_list) {
-				list_del(&cfid->entry);
-				cfid->on_list = false;
-				cfids->num_entries--;
-			}
-			rc = -ENOENT;
-		} else {
+	if (rc) {
+		spin_lock(&cfids->cfid_list_lock);
+		if (cfid->on_list) {
+			list_del(&cfid->entry);
+			cfid->on_list = false;
+			cfids->num_entries--;
+		}
+		if (cfid->has_lease) {
 			/*
 			 * We are guaranteed to have two references at this
 			 * point. One for the caller and one for a potential
@@ -327,25 +342,24 @@ int open_cached_dir(unsigned int xid, struct cifs_tcon *tcon,
 			 * will be closed when the caller closes the cached
 			 * handle.
 			 */
+			cfid->has_lease = false;
 			spin_unlock(&cfids->cfid_list_lock);
 			kref_put(&cfid->refcount, smb2_close_cached_fid);
 			goto out;
 		}
+		spin_unlock(&cfids->cfid_list_lock);
 	}
-	spin_unlock(&cfids->cfid_list_lock);
+out:
 	if (rc) {
 		if (cfid->is_open)
 			SMB2_close(0, cfid->tcon, cfid->fid.persistent_fid,
 				   cfid->fid.volatile_fid);
 		free_cached_dir(cfid);
-		cfid = NULL;
-	}
-out:
-	if (rc == 0) {
+	} else {
 		*ret_cfid = cfid;
 		atomic_inc(&tcon->num_remote_opens);
 	}
-
+	kfree(utf16_path);
 	return rc;
 }
 
-- 
2.42.0

Re: [PATCH 4/4] smb: client: fix use-after-free in smb2_query_info_compound()

[Steve French]

added Cc: stable and tentatively merged this to for-next

On Mon, Oct 30, 2023 at 3:20 PM Paulo Alcantara <pc@manguebit.com> wrote:
>
> The following UAF was triggered when running fstests generic/072 with
> KASAN enabled against Windows Server 2022 and mount options
> 'multichannel,max_channels=2,vers=3.1.1,mfsymlinks,noperm'
>
>   BUG: KASAN: slab-use-after-free in smb2_query_info_compound+0x423/0x6d0 [cifs]
>   Read of size 8 at addr ffff888014941048 by task xfs_io/27534
>
>   CPU: 0 PID: 27534 Comm: xfs_io Not tainted 6.6.0-rc7 #1
>   Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS
>   rel-1.16.2-3-gd478f380-rebuilt.opensuse.org 04/01/2014
>   Call Trace:
>    dump_stack_lvl+0x4a/0x80
>    print_report+0xcf/0x650
>    ? srso_alias_return_thunk+0x5/0x7f
>    ? srso_alias_return_thunk+0x5/0x7f
>    ? __phys_addr+0x46/0x90
>    kasan_report+0xda/0x110
>    ? smb2_query_info_compound+0x423/0x6d0 [cifs]
>    ? smb2_query_info_compound+0x423/0x6d0 [cifs]
>    smb2_query_info_compound+0x423/0x6d0 [cifs]
>    ? __pfx_smb2_query_info_compound+0x10/0x10 [cifs]
>    ? srso_alias_return_thunk+0x5/0x7f
>    ? __stack_depot_save+0x39/0x480
>    ? kasan_save_stack+0x33/0x60
>    ? kasan_set_track+0x25/0x30
>    ? ____kasan_slab_free+0x126/0x170
>    smb2_queryfs+0xc2/0x2c0 [cifs]
>    ? __pfx_smb2_queryfs+0x10/0x10 [cifs]
>    ? __pfx___lock_acquire+0x10/0x10
>    smb311_queryfs+0x210/0x220 [cifs]
>    ? __pfx_smb311_queryfs+0x10/0x10 [cifs]
>    ? srso_alias_return_thunk+0x5/0x7f
>    ? __lock_acquire+0x480/0x26c0
>    ? lock_release+0x1ed/0x640
>    ? srso_alias_return_thunk+0x5/0x7f
>    ? do_raw_spin_unlock+0x9b/0x100
>    cifs_statfs+0x18c/0x4b0 [cifs]
>    statfs_by_dentry+0x9b/0xf0
>    fd_statfs+0x4e/0xb0
>    __do_sys_fstatfs+0x7f/0xe0
>    ? __pfx___do_sys_fstatfs+0x10/0x10
>    ? srso_alias_return_thunk+0x5/0x7f
>    ? lockdep_hardirqs_on_prepare+0x136/0x200
>    ? srso_alias_return_thunk+0x5/0x7f
>    do_syscall_64+0x3f/0x90
>    entry_SYSCALL_64_after_hwframe+0x6e/0xd8
>
>   Allocated by task 27534:
>    kasan_save_stack+0x33/0x60
>    kasan_set_track+0x25/0x30
>    __kasan_kmalloc+0x8f/0xa0
>    open_cached_dir+0x71b/0x1240 [cifs]
>    smb2_query_info_compound+0x5c3/0x6d0 [cifs]
>    smb2_queryfs+0xc2/0x2c0 [cifs]
>    smb311_queryfs+0x210/0x220 [cifs]
>    cifs_statfs+0x18c/0x4b0 [cifs]
>    statfs_by_dentry+0x9b/0xf0
>    fd_statfs+0x4e/0xb0
>    __do_sys_fstatfs+0x7f/0xe0
>    do_syscall_64+0x3f/0x90
>    entry_SYSCALL_64_after_hwframe+0x6e/0xd8
>
>   Freed by task 27534:
>    kasan_save_stack+0x33/0x60
>    kasan_set_track+0x25/0x30
>    kasan_save_free_info+0x2b/0x50
>    ____kasan_slab_free+0x126/0x170
>    slab_free_freelist_hook+0xd0/0x1e0
>    __kmem_cache_free+0x9d/0x1b0
>    open_cached_dir+0xff5/0x1240 [cifs]
>    smb2_query_info_compound+0x5c3/0x6d0 [cifs]
>    smb2_queryfs+0xc2/0x2c0 [cifs]
>
> This is a race between open_cached_dir() and cached_dir_lease_break()
> where the cache entry for the open directory handle receives a lease
> break while creating it.  And before returning from open_cached_dir(),
> we put the last reference of the new @cfid because of
> !@cfid->has_lease.
>
> Besides the UAF, while running xfstests a lot of missed lease breaks
> have been noticed in tests that run several concurrent statfs(2) calls
> on those cached fids
>
>   CIFS: VFS: \\w22-root1.gandalf.test No task to wake, unknown frame...
>   CIFS: VFS: \\w22-root1.gandalf.test Cmd: 18 Err: 0x0 Flags: 0x1...
>   CIFS: VFS: \\w22-root1.gandalf.test smb buf 00000000715bfe83 len 108
>   CIFS: VFS: Dump pending requests:
>   CIFS: VFS: \\w22-root1.gandalf.test No task to wake, unknown frame...
>   CIFS: VFS: \\w22-root1.gandalf.test Cmd: 18 Err: 0x0 Flags: 0x1...
>   CIFS: VFS: \\w22-root1.gandalf.test smb buf 000000005aa7316e len 108
>   ...
>
> To fix both, in open_cached_dir() ensure that @cfid->has_lease is set
> right before sending out compounded request so that any potential
> lease break will be get processed by demultiplex thread while we're
> still caching @cfid.  And, if open failed for some reason, re-check
> @cfid->has_lease to decide whether or not put lease reference.
>
> Signed-off-by: Paulo Alcantara (SUSE) <pc@manguebit.com>
> ---
>  fs/smb/client/cached_dir.c | 84 ++++++++++++++++++++++----------------
>  1 file changed, 49 insertions(+), 35 deletions(-)
>
> diff --git a/fs/smb/client/cached_dir.c b/fs/smb/client/cached_dir.c
> index fe1bf5b6e0cb..59f6b8e32cc9 100644
> --- a/fs/smb/client/cached_dir.c
> +++ b/fs/smb/client/cached_dir.c
> @@ -32,7 +32,7 @@ static struct cached_fid *find_or_create_cached_dir(struct cached_fids *cfids,
>                          * fully cached or it may be in the process of
>                          * being deleted due to a lease break.
>                          */
> -                       if (!cfid->has_lease) {
> +                       if (!cfid->time || !cfid->has_lease) {
>                                 spin_unlock(&cfids->cfid_list_lock);
>                                 return NULL;
>                         }
> @@ -193,10 +193,20 @@ int open_cached_dir(unsigned int xid, struct cifs_tcon *tcon,
>         npath = path_no_prefix(cifs_sb, path);
>         if (IS_ERR(npath)) {
>                 rc = PTR_ERR(npath);
> -               kfree(utf16_path);
> -               return rc;
> +               goto out;
>         }
>
> +       if (!npath[0]) {
> +               dentry = dget(cifs_sb->root);
> +       } else {
> +               dentry = path_to_dentry(cifs_sb, npath);
> +               if (IS_ERR(dentry)) {
> +                       rc = -ENOENT;
> +                       goto out;
> +               }
> +       }
> +       cfid->dentry = dentry;
> +
>         /*
>          * We do not hold the lock for the open because in case
>          * SMB2_open needs to reconnect.
> @@ -249,6 +259,15 @@ int open_cached_dir(unsigned int xid, struct cifs_tcon *tcon,
>
>         smb2_set_related(&rqst[1]);
>
> +       /*
> +        * Set @cfid->has_lease to true before sending out compounded request so
> +        * its lease reference can be put in cached_dir_lease_break() due to a
> +        * potential lease break right after the request is sent or while @cfid
> +        * is still being cached.  Concurrent processes won't be to use it yet
> +        * due to @cfid->time being zero.
> +        */
> +       cfid->has_lease = true;
> +
>         rc = compound_send_recv(xid, ses, server,
>                                 flags, 2, rqst,
>                                 resp_buftype, rsp_iov);
> @@ -263,6 +282,8 @@ int open_cached_dir(unsigned int xid, struct cifs_tcon *tcon,
>         cfid->tcon = tcon;
>         cfid->is_open = true;
>
> +       spin_lock(&cfids->cfid_list_lock);
> +
>         o_rsp = (struct smb2_create_rsp *)rsp_iov[0].iov_base;
>         oparms.fid->persistent_fid = o_rsp->PersistentFileId;
>         oparms.fid->volatile_fid = o_rsp->VolatileFileId;
> @@ -270,18 +291,25 @@ int open_cached_dir(unsigned int xid, struct cifs_tcon *tcon,
>         oparms.fid->mid = le64_to_cpu(o_rsp->hdr.MessageId);
>  #endif /* CIFS_DEBUG2 */
>
> -       if (o_rsp->OplockLevel != SMB2_OPLOCK_LEVEL_LEASE)
> +       rc = -EINVAL;
> +       if (o_rsp->OplockLevel != SMB2_OPLOCK_LEVEL_LEASE) {
> +               spin_unlock(&cfids->cfid_list_lock);
>                 goto oshr_free;
> +       }
>
>         smb2_parse_contexts(server, o_rsp,
>                             &oparms.fid->epoch,
>                             oparms.fid->lease_key, &oplock,
>                             NULL, NULL);
> -       if (!(oplock & SMB2_LEASE_READ_CACHING_HE))
> +       if (!(oplock & SMB2_LEASE_READ_CACHING_HE)) {
> +               spin_unlock(&cfids->cfid_list_lock);
>                 goto oshr_free;
> +       }
>         qi_rsp = (struct smb2_query_info_rsp *)rsp_iov[1].iov_base;
> -       if (le32_to_cpu(qi_rsp->OutputBufferLength) < sizeof(struct smb2_file_all_info))
> +       if (le32_to_cpu(qi_rsp->OutputBufferLength) < sizeof(struct smb2_file_all_info)) {
> +               spin_unlock(&cfids->cfid_list_lock);
>                 goto oshr_free;
> +       }
>         if (!smb2_validate_and_copy_iov(
>                                 le16_to_cpu(qi_rsp->OutputBufferOffset),
>                                 sizeof(struct smb2_file_all_info),
> @@ -289,37 +317,24 @@ int open_cached_dir(unsigned int xid, struct cifs_tcon *tcon,
>                                 (char *)&cfid->file_all_info))
>                 cfid->file_all_info_is_valid = true;
>
> -       if (!npath[0])
> -               dentry = dget(cifs_sb->root);
> -       else {
> -               dentry = path_to_dentry(cifs_sb, npath);
> -               if (IS_ERR(dentry)) {
> -                       rc = -ENOENT;
> -                       goto oshr_free;
> -               }
> -       }
> -       spin_lock(&cfids->cfid_list_lock);
> -       cfid->dentry = dentry;
>         cfid->time = jiffies;
> -       cfid->has_lease = true;
>         spin_unlock(&cfids->cfid_list_lock);
> +       /* At this point the directory handle is fully cached */
> +       rc = 0;
>
>  oshr_free:
> -       kfree(utf16_path);
>         SMB2_open_free(&rqst[0]);
>         SMB2_query_info_free(&rqst[1]);
>         free_rsp_buf(resp_buftype[0], rsp_iov[0].iov_base);
>         free_rsp_buf(resp_buftype[1], rsp_iov[1].iov_base);
> -       spin_lock(&cfids->cfid_list_lock);
> -       if (!cfid->has_lease) {
> -               if (rc) {
> -                       if (cfid->on_list) {
> -                               list_del(&cfid->entry);
> -                               cfid->on_list = false;
> -                               cfids->num_entries--;
> -                       }
> -                       rc = -ENOENT;
> -               } else {
> +       if (rc) {
> +               spin_lock(&cfids->cfid_list_lock);
> +               if (cfid->on_list) {
> +                       list_del(&cfid->entry);
> +                       cfid->on_list = false;
> +                       cfids->num_entries--;
> +               }
> +               if (cfid->has_lease) {
>                         /*
>                          * We are guaranteed to have two references at this
>                          * point. One for the caller and one for a potential
> @@ -327,25 +342,24 @@ int open_cached_dir(unsigned int xid, struct cifs_tcon *tcon,
>                          * will be closed when the caller closes the cached
>                          * handle.
>                          */
> +                       cfid->has_lease = false;
>                         spin_unlock(&cfids->cfid_list_lock);
>                         kref_put(&cfid->refcount, smb2_close_cached_fid);
>                         goto out;
>                 }
> +               spin_unlock(&cfids->cfid_list_lock);
>         }
> -       spin_unlock(&cfids->cfid_list_lock);
> +out:
>         if (rc) {
>                 if (cfid->is_open)
>                         SMB2_close(0, cfid->tcon, cfid->fid.persistent_fid,
>                                    cfid->fid.volatile_fid);
>                 free_cached_dir(cfid);
> -               cfid = NULL;
> -       }
> -out:
> -       if (rc == 0) {
> +       } else {
>                 *ret_cfid = cfid;
>                 atomic_inc(&tcon->num_remote_opens);
>         }
> -
> +       kfree(utf16_path);
>         return rc;
>  }
>
> --
> 2.42.0
>


-- 
Thanks,

Steve

Re: [PATCH 4/4] smb: client: fix use-after-free in smb2_query_info_compound()

[Shyam Prasad N]

On Tue, Oct 31, 2023 at 8:39 AM Steve French <smfrench@gmail.com> wrote:
>
> added Cc: stable and tentatively merged this to for-next
>
> On Mon, Oct 30, 2023 at 3:20 PM Paulo Alcantara <pc@manguebit.com> wrote:
> >
> > The following UAF was triggered when running fstests generic/072 with
> > KASAN enabled against Windows Server 2022 and mount options
> > 'multichannel,max_channels=2,vers=3.1.1,mfsymlinks,noperm'
> >
> >   BUG: KASAN: slab-use-after-free in smb2_query_info_compound+0x423/0x6d0 [cifs]
> >   Read of size 8 at addr ffff888014941048 by task xfs_io/27534
> >
> >   CPU: 0 PID: 27534 Comm: xfs_io Not tainted 6.6.0-rc7 #1
> >   Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS
> >   rel-1.16.2-3-gd478f380-rebuilt.opensuse.org 04/01/2014
> >   Call Trace:
> >    dump_stack_lvl+0x4a/0x80
> >    print_report+0xcf/0x650
> >    ? srso_alias_return_thunk+0x5/0x7f
> >    ? srso_alias_return_thunk+0x5/0x7f
> >    ? __phys_addr+0x46/0x90
> >    kasan_report+0xda/0x110
> >    ? smb2_query_info_compound+0x423/0x6d0 [cifs]
> >    ? smb2_query_info_compound+0x423/0x6d0 [cifs]
> >    smb2_query_info_compound+0x423/0x6d0 [cifs]
> >    ? __pfx_smb2_query_info_compound+0x10/0x10 [cifs]
> >    ? srso_alias_return_thunk+0x5/0x7f
> >    ? __stack_depot_save+0x39/0x480
> >    ? kasan_save_stack+0x33/0x60
> >    ? kasan_set_track+0x25/0x30
> >    ? ____kasan_slab_free+0x126/0x170
> >    smb2_queryfs+0xc2/0x2c0 [cifs]
> >    ? __pfx_smb2_queryfs+0x10/0x10 [cifs]
> >    ? __pfx___lock_acquire+0x10/0x10
> >    smb311_queryfs+0x210/0x220 [cifs]
> >    ? __pfx_smb311_queryfs+0x10/0x10 [cifs]
> >    ? srso_alias_return_thunk+0x5/0x7f
> >    ? __lock_acquire+0x480/0x26c0
> >    ? lock_release+0x1ed/0x640
> >    ? srso_alias_return_thunk+0x5/0x7f
> >    ? do_raw_spin_unlock+0x9b/0x100
> >    cifs_statfs+0x18c/0x4b0 [cifs]
> >    statfs_by_dentry+0x9b/0xf0
> >    fd_statfs+0x4e/0xb0
> >    __do_sys_fstatfs+0x7f/0xe0
> >    ? __pfx___do_sys_fstatfs+0x10/0x10
> >    ? srso_alias_return_thunk+0x5/0x7f
> >    ? lockdep_hardirqs_on_prepare+0x136/0x200
> >    ? srso_alias_return_thunk+0x5/0x7f
> >    do_syscall_64+0x3f/0x90
> >    entry_SYSCALL_64_after_hwframe+0x6e/0xd8
> >
> >   Allocated by task 27534:
> >    kasan_save_stack+0x33/0x60
> >    kasan_set_track+0x25/0x30
> >    __kasan_kmalloc+0x8f/0xa0
> >    open_cached_dir+0x71b/0x1240 [cifs]
> >    smb2_query_info_compound+0x5c3/0x6d0 [cifs]
> >    smb2_queryfs+0xc2/0x2c0 [cifs]
> >    smb311_queryfs+0x210/0x220 [cifs]
> >    cifs_statfs+0x18c/0x4b0 [cifs]
> >    statfs_by_dentry+0x9b/0xf0
> >    fd_statfs+0x4e/0xb0
> >    __do_sys_fstatfs+0x7f/0xe0
> >    do_syscall_64+0x3f/0x90
> >    entry_SYSCALL_64_after_hwframe+0x6e/0xd8
> >
> >   Freed by task 27534:
> >    kasan_save_stack+0x33/0x60
> >    kasan_set_track+0x25/0x30
> >    kasan_save_free_info+0x2b/0x50
> >    ____kasan_slab_free+0x126/0x170
> >    slab_free_freelist_hook+0xd0/0x1e0
> >    __kmem_cache_free+0x9d/0x1b0
> >    open_cached_dir+0xff5/0x1240 [cifs]
> >    smb2_query_info_compound+0x5c3/0x6d0 [cifs]
> >    smb2_queryfs+0xc2/0x2c0 [cifs]
> >
> > This is a race between open_cached_dir() and cached_dir_lease_break()
> > where the cache entry for the open directory handle receives a lease
> > break while creating it.  And before returning from open_cached_dir(),
> > we put the last reference of the new @cfid because of
> > !@cfid->has_lease.
> >
> > Besides the UAF, while running xfstests a lot of missed lease breaks
> > have been noticed in tests that run several concurrent statfs(2) calls
> > on those cached fids
> >
> >   CIFS: VFS: \\w22-root1.gandalf.test No task to wake, unknown frame...
> >   CIFS: VFS: \\w22-root1.gandalf.test Cmd: 18 Err: 0x0 Flags: 0x1...
> >   CIFS: VFS: \\w22-root1.gandalf.test smb buf 00000000715bfe83 len 108
> >   CIFS: VFS: Dump pending requests:
> >   CIFS: VFS: \\w22-root1.gandalf.test No task to wake, unknown frame...
> >   CIFS: VFS: \\w22-root1.gandalf.test Cmd: 18 Err: 0x0 Flags: 0x1...
> >   CIFS: VFS: \\w22-root1.gandalf.test smb buf 000000005aa7316e len 108
> >   ...
> >
> > To fix both, in open_cached_dir() ensure that @cfid->has_lease is set
> > right before sending out compounded request so that any potential
> > lease break will be get processed by demultiplex thread while we're
> > still caching @cfid.  And, if open failed for some reason, re-check
> > @cfid->has_lease to decide whether or not put lease reference.
> >
> > Signed-off-by: Paulo Alcantara (SUSE) <pc@manguebit.com>
> > ---
> >  fs/smb/client/cached_dir.c | 84 ++++++++++++++++++++++----------------
> >  1 file changed, 49 insertions(+), 35 deletions(-)
> >
> > diff --git a/fs/smb/client/cached_dir.c b/fs/smb/client/cached_dir.c
> > index fe1bf5b6e0cb..59f6b8e32cc9 100644
> > --- a/fs/smb/client/cached_dir.c
> > +++ b/fs/smb/client/cached_dir.c
> > @@ -32,7 +32,7 @@ static struct cached_fid *find_or_create_cached_dir(struct cached_fids *cfids,
> >                          * fully cached or it may be in the process of
> >                          * being deleted due to a lease break.
> >                          */
> > -                       if (!cfid->has_lease) {
> > +                       if (!cfid->time || !cfid->has_lease) {
> >                                 spin_unlock(&cfids->cfid_list_lock);
> >                                 return NULL;
> >                         }
> > @@ -193,10 +193,20 @@ int open_cached_dir(unsigned int xid, struct cifs_tcon *tcon,
> >         npath = path_no_prefix(cifs_sb, path);
> >         if (IS_ERR(npath)) {
> >                 rc = PTR_ERR(npath);
> > -               kfree(utf16_path);
> > -               return rc;
> > +               goto out;
> >         }
> >
> > +       if (!npath[0]) {
> > +               dentry = dget(cifs_sb->root);
> > +       } else {
> > +               dentry = path_to_dentry(cifs_sb, npath);
> > +               if (IS_ERR(dentry)) {
> > +                       rc = -ENOENT;
> > +                       goto out;
> > +               }
> > +       }
> > +       cfid->dentry = dentry;
> > +
> >         /*
> >          * We do not hold the lock for the open because in case
> >          * SMB2_open needs to reconnect.
> > @@ -249,6 +259,15 @@ int open_cached_dir(unsigned int xid, struct cifs_tcon *tcon,
> >
> >         smb2_set_related(&rqst[1]);
> >
> > +       /*
> > +        * Set @cfid->has_lease to true before sending out compounded request so
> > +        * its lease reference can be put in cached_dir_lease_break() due to a
> > +        * potential lease break right after the request is sent or while @cfid
> > +        * is still being cached.  Concurrent processes won't be to use it yet
> > +        * due to @cfid->time being zero.
> > +        */
> > +       cfid->has_lease = true;
> > +
> >         rc = compound_send_recv(xid, ses, server,
> >                                 flags, 2, rqst,
> >                                 resp_buftype, rsp_iov);
> > @@ -263,6 +282,8 @@ int open_cached_dir(unsigned int xid, struct cifs_tcon *tcon,
> >         cfid->tcon = tcon;
> >         cfid->is_open = true;
> >
> > +       spin_lock(&cfids->cfid_list_lock);
> > +
> >         o_rsp = (struct smb2_create_rsp *)rsp_iov[0].iov_base;
> >         oparms.fid->persistent_fid = o_rsp->PersistentFileId;
> >         oparms.fid->volatile_fid = o_rsp->VolatileFileId;
> > @@ -270,18 +291,25 @@ int open_cached_dir(unsigned int xid, struct cifs_tcon *tcon,
> >         oparms.fid->mid = le64_to_cpu(o_rsp->hdr.MessageId);
> >  #endif /* CIFS_DEBUG2 */
> >
> > -       if (o_rsp->OplockLevel != SMB2_OPLOCK_LEVEL_LEASE)
> > +       rc = -EINVAL;
> > +       if (o_rsp->OplockLevel != SMB2_OPLOCK_LEVEL_LEASE) {
> > +               spin_unlock(&cfids->cfid_list_lock);
> >                 goto oshr_free;
> > +       }
> >
> >         smb2_parse_contexts(server, o_rsp,
> >                             &oparms.fid->epoch,
> >                             oparms.fid->lease_key, &oplock,
> >                             NULL, NULL);
> > -       if (!(oplock & SMB2_LEASE_READ_CACHING_HE))
> > +       if (!(oplock & SMB2_LEASE_READ_CACHING_HE)) {
> > +               spin_unlock(&cfids->cfid_list_lock);
> >                 goto oshr_free;
> > +       }
> >         qi_rsp = (struct smb2_query_info_rsp *)rsp_iov[1].iov_base;
> > -       if (le32_to_cpu(qi_rsp->OutputBufferLength) < sizeof(struct smb2_file_all_info))
> > +       if (le32_to_cpu(qi_rsp->OutputBufferLength) < sizeof(struct smb2_file_all_info)) {
> > +               spin_unlock(&cfids->cfid_list_lock);
> >                 goto oshr_free;
> > +       }
> >         if (!smb2_validate_and_copy_iov(
> >                                 le16_to_cpu(qi_rsp->OutputBufferOffset),
> >                                 sizeof(struct smb2_file_all_info),
> > @@ -289,37 +317,24 @@ int open_cached_dir(unsigned int xid, struct cifs_tcon *tcon,
> >                                 (char *)&cfid->file_all_info))
> >                 cfid->file_all_info_is_valid = true;
> >
> > -       if (!npath[0])
> > -               dentry = dget(cifs_sb->root);
> > -       else {
> > -               dentry = path_to_dentry(cifs_sb, npath);
> > -               if (IS_ERR(dentry)) {
> > -                       rc = -ENOENT;
> > -                       goto oshr_free;
> > -               }
> > -       }
> > -       spin_lock(&cfids->cfid_list_lock);
> > -       cfid->dentry = dentry;
> >         cfid->time = jiffies;
> > -       cfid->has_lease = true;
> >         spin_unlock(&cfids->cfid_list_lock);
> > +       /* At this point the directory handle is fully cached */
> > +       rc = 0;
> >
> >  oshr_free:
> > -       kfree(utf16_path);
> >         SMB2_open_free(&rqst[0]);
> >         SMB2_query_info_free(&rqst[1]);
> >         free_rsp_buf(resp_buftype[0], rsp_iov[0].iov_base);
> >         free_rsp_buf(resp_buftype[1], rsp_iov[1].iov_base);
> > -       spin_lock(&cfids->cfid_list_lock);
> > -       if (!cfid->has_lease) {
> > -               if (rc) {
> > -                       if (cfid->on_list) {
> > -                               list_del(&cfid->entry);
> > -                               cfid->on_list = false;
> > -                               cfids->num_entries--;
> > -                       }
> > -                       rc = -ENOENT;
> > -               } else {
> > +       if (rc) {
> > +               spin_lock(&cfids->cfid_list_lock);
> > +               if (cfid->on_list) {
> > +                       list_del(&cfid->entry);
> > +                       cfid->on_list = false;
> > +                       cfids->num_entries--;
> > +               }
> > +               if (cfid->has_lease) {
> >                         /*
> >                          * We are guaranteed to have two references at this
> >                          * point. One for the caller and one for a potential
> > @@ -327,25 +342,24 @@ int open_cached_dir(unsigned int xid, struct cifs_tcon *tcon,
> >                          * will be closed when the caller closes the cached
> >                          * handle.
> >                          */
> > +                       cfid->has_lease = false;
> >                         spin_unlock(&cfids->cfid_list_lock);
> >                         kref_put(&cfid->refcount, smb2_close_cached_fid);
> >                         goto out;
> >                 }
> > +               spin_unlock(&cfids->cfid_list_lock);
> >         }
> > -       spin_unlock(&cfids->cfid_list_lock);
> > +out:
> >         if (rc) {
> >                 if (cfid->is_open)
> >                         SMB2_close(0, cfid->tcon, cfid->fid.persistent_fid,
> >                                    cfid->fid.volatile_fid);
> >                 free_cached_dir(cfid);
> > -               cfid = NULL;
> > -       }
> > -out:
> > -       if (rc == 0) {
> > +       } else {
> >                 *ret_cfid = cfid;
> >                 atomic_inc(&tcon->num_remote_opens);
> >         }
> > -
> > +       kfree(utf16_path);
> >         return rc;
> >  }
> >
> > --
> > 2.42.0
> >
>
>
> --
> Thanks,
>
> Steve

Don't know if I'm too late for the review.
If not, you can add my RB.

-- 
Regards,
Shyam

Re: [PATCH 1/4] smb: client: remove extra @chan_count check in __cifs_put_smb_ses()

[Steve French]

tentatively merged into cifs-2.6.git for-next pending review/testing

On Mon, Oct 30, 2023 at 3:20 PM Paulo Alcantara <pc@manguebit.com> wrote:
>
> If @ses->chan_count <= 1, then for-loop body will not be executed so
> no need to check it twice.
>
> Signed-off-by: Paulo Alcantara (SUSE) <pc@manguebit.com>
> ---
>  fs/smb/client/connect.c | 23 +++++++++--------------
>  1 file changed, 9 insertions(+), 14 deletions(-)
>
> diff --git a/fs/smb/client/connect.c b/fs/smb/client/connect.c
> index 7b923e36501b..a017ee552256 100644
> --- a/fs/smb/client/connect.c
> +++ b/fs/smb/client/connect.c
> @@ -1969,9 +1969,10 @@ cifs_find_smb_ses(struct TCP_Server_Info *server, struct smb3_fs_context *ctx)
>
>  void __cifs_put_smb_ses(struct cifs_ses *ses)
>  {
> -       unsigned int rc, xid;
> -       unsigned int chan_count;
>         struct TCP_Server_Info *server = ses->server;
> +       unsigned int xid;
> +       size_t i;
> +       int rc;
>
>         spin_lock(&ses->ses_lock);
>         if (ses->ses_status == SES_EXITING) {
> @@ -2017,20 +2018,14 @@ void __cifs_put_smb_ses(struct cifs_ses *ses)
>         list_del_init(&ses->smb_ses_list);
>         spin_unlock(&cifs_tcp_ses_lock);
>
> -       chan_count = ses->chan_count;
> -
>         /* close any extra channels */
> -       if (chan_count > 1) {
> -               int i;
> -
> -               for (i = 1; i < chan_count; i++) {
> -                       if (ses->chans[i].iface) {
> -                               kref_put(&ses->chans[i].iface->refcount, release_iface);
> -                               ses->chans[i].iface = NULL;
> -                       }
> -                       cifs_put_tcp_session(ses->chans[i].server, 0);
> -                       ses->chans[i].server = NULL;
> +       for (i = 1; i < ses->chan_count; i++) {
> +               if (ses->chans[i].iface) {
> +                       kref_put(&ses->chans[i].iface->refcount, release_iface);
> +                       ses->chans[i].iface = NULL;
>                 }
> +               cifs_put_tcp_session(ses->chans[i].server, 0);
> +               ses->chans[i].server = NULL;
>         }
>
>         sesInfoFree(ses);
> --
> 2.42.0
>


-- 
Thanks,

Steve

Re: [PATCH 1/4] smb: client: remove extra @chan_count check in __cifs_put_smb_ses()

[Shyam Prasad N]

On Tue, Oct 31, 2023 at 8:54 AM Steve French <smfrench@gmail.com> wrote:
>
> tentatively merged into cifs-2.6.git for-next pending review/testing
>
> On Mon, Oct 30, 2023 at 3:20 PM Paulo Alcantara <pc@manguebit.com> wrote:
> >
> > If @ses->chan_count <= 1, then for-loop body will not be executed so
> > no need to check it twice.
> >
> > Signed-off-by: Paulo Alcantara (SUSE) <pc@manguebit.com>
> > ---
> >  fs/smb/client/connect.c | 23 +++++++++--------------
> >  1 file changed, 9 insertions(+), 14 deletions(-)
> >
> > diff --git a/fs/smb/client/connect.c b/fs/smb/client/connect.c
> > index 7b923e36501b..a017ee552256 100644
> > --- a/fs/smb/client/connect.c
> > +++ b/fs/smb/client/connect.c
> > @@ -1969,9 +1969,10 @@ cifs_find_smb_ses(struct TCP_Server_Info *server, struct smb3_fs_context *ctx)
> >
> >  void __cifs_put_smb_ses(struct cifs_ses *ses)
> >  {
> > -       unsigned int rc, xid;
> > -       unsigned int chan_count;
> >         struct TCP_Server_Info *server = ses->server;
> > +       unsigned int xid;
> > +       size_t i;
> > +       int rc;
> >
> >         spin_lock(&ses->ses_lock);
> >         if (ses->ses_status == SES_EXITING) {
> > @@ -2017,20 +2018,14 @@ void __cifs_put_smb_ses(struct cifs_ses *ses)
> >         list_del_init(&ses->smb_ses_list);
> >         spin_unlock(&cifs_tcp_ses_lock);
> >
> > -       chan_count = ses->chan_count;
> > -
> >         /* close any extra channels */
> > -       if (chan_count > 1) {
> > -               int i;
> > -
> > -               for (i = 1; i < chan_count; i++) {
> > -                       if (ses->chans[i].iface) {
> > -                               kref_put(&ses->chans[i].iface->refcount, release_iface);
> > -                               ses->chans[i].iface = NULL;
> > -                       }
> > -                       cifs_put_tcp_session(ses->chans[i].server, 0);
> > -                       ses->chans[i].server = NULL;
> > +       for (i = 1; i < ses->chan_count; i++) {
> > +               if (ses->chans[i].iface) {
> > +                       kref_put(&ses->chans[i].iface->refcount, release_iface);
> > +                       ses->chans[i].iface = NULL;
> >                 }
> > +               cifs_put_tcp_session(ses->chans[i].server, 0);
> > +               ses->chans[i].server = NULL;
> >         }
> >
> >         sesInfoFree(ses);
> > --
> > 2.42.0
> >
>
>
> --
> Thanks,
>
> Steve

Looks good to me.
You can add my RB.

-- 
Regards,
Shyam