| Message ID | 20250311013231.2684868-1-henrique.carvalho@suse.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | smb: client: Fix match_session bug causing duplicate session creation | expand |
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 > >
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
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
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)
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(-)