[PATCH] smb: client: Fix match_session bug causing duplicate session creation

[PATCH] smb: client: Fix match_session bug causing duplicate session creation

[Henrique Carvalho]

Fix a bug in match_session() that can result in duplicate sessions being
created even when the session data is identical.

match_session() compares ctx->sectype against ses->sectype only. This is
flawed because ses->sectype could be Unspecified while ctx->sectype
could be the same selected security type for the compared session. This
causes the function to mismatch the potential same session, resulting in
two of the same sessions.

Reproduction steps:

mount.cifs //server/share /mnt/a -o credentials=creds
mount.cifs //server/share /mnt/b -o credentials=creds,sec=ntlmssp
cat /proc/fs/cifs/DebugData | grep SessionId | wc -l  # output is 1

mount.cifs //server/share /mnt/b -o credentials=creds,sec=ntlmssp
mount.cifs //server/share /mnt/a -o credentials=creds
cat /proc/fs/cifs/DebugData | grep SessionId | wc -l  # output is 2

Fixes: 3f618223dc0bd ("move sectype to the cifs_ses instead of
TCP_Server_Info")
Signed-off-by: Henrique Carvalho <henrique.carvalho@suse.com>
---
 fs/smb/client/connect.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/fs/smb/client/connect.c b/fs/smb/client/connect.c
index f917de020dd5..0c8c523d52be 100644
--- a/fs/smb/client/connect.c
+++ b/fs/smb/client/connect.c
@@ -1825,8 +1825,11 @@ static int match_session(struct cifs_ses *ses,
 			 struct smb3_fs_context *ctx,
 			 bool match_super)
 {
+	struct TCP_Server_Info *server = ses->server;
+	enum securityEnum selected_sectype = server->ops->select_sectype(ses->server, ctx->sectype);
+
 	if (ctx->sectype != Unspecified &&
-	    ctx->sectype != ses->sectype)
+	    ctx->sectype != selected_sectype)
 		return 0;
 
 	if (!match_super && ctx->dfs_root_ses != ses->dfs_root_ses)
-- 
2.47.0

Re: [PATCH] smb: client: Fix match_session bug causing duplicate session creation

[Steve French]

merged into cifs-2.6.git for-next pending additional reviews and testing

On Mon, Mar 10, 2025 at 8:35 PM Henrique Carvalho
<henrique.carvalho@suse.com> wrote:
>
> Fix a bug in match_session() that can result in duplicate sessions being
> created even when the session data is identical.
>
> match_session() compares ctx->sectype against ses->sectype only. This is
> flawed because ses->sectype could be Unspecified while ctx->sectype
> could be the same selected security type for the compared session. This
> causes the function to mismatch the potential same session, resulting in
> two of the same sessions.
>
> Reproduction steps:
>
> mount.cifs //server/share /mnt/a -o credentials=creds
> mount.cifs //server/share /mnt/b -o credentials=creds,sec=ntlmssp
> cat /proc/fs/cifs/DebugData | grep SessionId | wc -l  # output is 1
>
> mount.cifs //server/share /mnt/b -o credentials=creds,sec=ntlmssp
> mount.cifs //server/share /mnt/a -o credentials=creds
> cat /proc/fs/cifs/DebugData | grep SessionId | wc -l  # output is 2
>
> Fixes: 3f618223dc0bd ("move sectype to the cifs_ses instead of
> TCP_Server_Info")
> Signed-off-by: Henrique Carvalho <henrique.carvalho@suse.com>
> ---
>  fs/smb/client/connect.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/fs/smb/client/connect.c b/fs/smb/client/connect.c
> index f917de020dd5..0c8c523d52be 100644
> --- a/fs/smb/client/connect.c
> +++ b/fs/smb/client/connect.c
> @@ -1825,8 +1825,11 @@ static int match_session(struct cifs_ses *ses,
>                          struct smb3_fs_context *ctx,
>                          bool match_super)
>  {
> +       struct TCP_Server_Info *server = ses->server;
> +       enum securityEnum selected_sectype = server->ops->select_sectype(ses->server, ctx->sectype);
> +
>         if (ctx->sectype != Unspecified &&
> -           ctx->sectype != ses->sectype)
> +           ctx->sectype != selected_sectype)
>                 return 0;
>
>         if (!match_super && ctx->dfs_root_ses != ses->dfs_root_ses)
> --
> 2.47.0
>
>


-- 
Thanks,

Steve

Re: [PATCH] smb: client: Fix match_session bug causing duplicate session creation

[Enzo Matsumiya]

Hi Henrique,

On 03/10, Henrique Carvalho wrote:
>Fix a bug in match_session() that can result in duplicate sessions being
>created even when the session data is identical.
>
>match_session() compares ctx->sectype against ses->sectype only. This is
>flawed because ses->sectype could be Unspecified while ctx->sectype
>could be the same selected security type for the compared session. This
>causes the function to mismatch the potential same session, resulting in
>two of the same sessions.
>
>Reproduction steps:
>
>mount.cifs //server/share /mnt/a -o credentials=creds
>mount.cifs //server/share /mnt/b -o credentials=creds,sec=ntlmssp
>cat /proc/fs/cifs/DebugData | grep SessionId | wc -l  # output is 1
>
>mount.cifs //server/share /mnt/b -o credentials=creds,sec=ntlmssp
>mount.cifs //server/share /mnt/a -o credentials=creds
>cat /proc/fs/cifs/DebugData | grep SessionId | wc -l  # output is 2

Minor nit: I think your reproducer results are inverted -- on my tests,
the session is reused when sec=ntlmssp is passed on first mount, not the
other way around.

>Fixes: 3f618223dc0bd ("move sectype to the cifs_ses instead of
>TCP_Server_Info")
>Signed-off-by: Henrique Carvalho <henrique.carvalho@suse.com>
>---
> fs/smb/client/connect.c | 5 ++++-
> 1 file changed, 4 insertions(+), 1 deletion(-)
>
>diff --git a/fs/smb/client/connect.c b/fs/smb/client/connect.c
>index f917de020dd5..0c8c523d52be 100644
>--- a/fs/smb/client/connect.c
>+++ b/fs/smb/client/connect.c
>@@ -1825,8 +1825,11 @@ static int match_session(struct cifs_ses *ses,
> 			 struct smb3_fs_context *ctx,
> 			 bool match_super)
> {
>+	struct TCP_Server_Info *server = ses->server;
>+	enum securityEnum selected_sectype = server->ops->select_sectype(ses->server, ctx->sectype);

Calling select_sectype() with ctx->sectype as argument works fine for
Unspecified and ntlmssp* cases (because Unspecified will default to
ntlmssp if server supports it), but if you do the first mount with
sec=krb5 and the second with sec=ntlmssp/no sec=, the krb5 session
will be reused (which is wrong).

I would suggest something like (quickly tested only):

{
-       if (ctx->sectype != Unspecified &&
-           ctx->sectype != ses->sectype)
+       struct TCP_Server_Info *server = ses->server;
+       enum securityEnum requested_sectype = server->ops->select_sectype(server, ctx->sectype),
+                         selected_sectype = server->ops->select_sectype(server, ses->sectype);
+
+       if (requested_sectype != selected_sectype)
		 return 0;

i.e. comparing what the new session would negotiate as sectype vs what
was negotiated for the previous session.

>+
> 	if (ctx->sectype != Unspecified &&
>-	    ctx->sectype != ses->sectype)
>+	    ctx->sectype != selected_sectype)
> 		return 0;
>
> 	if (!match_super && ctx->dfs_root_ses != ses->dfs_root_ses)
>-- 

Other than that,

Reviewed-by: Enzo Matsumiya <ematsumiya@suse.de>


Cheers,

Enzo

Re: [PATCH] smb: client: Fix match_session bug causing duplicate session creation

[Henrique Carvalho]

On Tue, Mar 11, 2025 at 10:39:37AM -0300, Enzo Matsumiya wrote:

> Minor nit: I think your reproducer results are inverted -- on my tests,
> the session is reused when sec=ntlmssp is passed on first mount, not the
> other way around.

> Calling select_sectype() with ctx->sectype as argument works fine for
> Unspecified and ntlmssp* cases (because Unspecified will default to
> ntlmssp if server supports it), but if you do the first mount with
> sec=krb5 and the second with sec=ntlmssp/no sec=, the krb5 session
> will be reused (which is wrong).

Both fixed on v2.

Thanks, Enzo.

Henrique