[PATCH 1/2] cifs: during remount, make sure passwords are in sync

[PATCH 1/2] cifs: during remount, make sure passwords are in sync

[meetakshisetiyaoss]

From: Shyam Prasad N <sprasad@microsoft.com>

We recently introduced a password2 field in both ses and ctx structs.
This was done so as to allow the client to rotate passwords for a mount
without any downtime. However, when the client transparently handles
password rotation, it can swap the values of the two password fields
in the ses struct, but not in smb3_fs_context struct that hangs off
cifs_sb. This can lead to a situation where a remount unintentionally
overwrites a working password in the ses struct.

In order to fix this, we first get the passwords in ctx struct
in-sync with ses struct, before replacing them with what the passwords
that could be passed as a part of remount.

Also, in order to avoid race condition between smb2_reconnect and
smb3_reconfigure, we make sure to lock session_mutex before changing
password and password2 fields of the ses structure.

Fixes: 35f834265e0d ("smb3: fix broken reconnect when password changing on the server by allowing password rotation")
Signed-off-by: Shyam Prasad N <sprasad@microsoft.com>
Signed-off-by: Meetakshi Setiya <msetiya@microsoft.com>
---
 fs/smb/client/fs_context.c | 69 +++++++++++++++++++++++++++++++++-----
 1 file changed, 60 insertions(+), 9 deletions(-)

diff --git a/fs/smb/client/fs_context.c b/fs/smb/client/fs_context.c
index 5c5a52019efa..73610e66c8d9 100644
--- a/fs/smb/client/fs_context.c
+++ b/fs/smb/client/fs_context.c
@@ -896,6 +896,7 @@ static int smb3_reconfigure(struct fs_context *fc)
 	struct dentry *root = fc->root;
 	struct cifs_sb_info *cifs_sb = CIFS_SB(root->d_sb);
 	struct cifs_ses *ses = cifs_sb_master_tcon(cifs_sb)->ses;
+	char *new_password = NULL, *new_password2 = NULL;
 	bool need_recon = false;
 	int rc;
 
@@ -915,21 +916,71 @@ static int smb3_reconfigure(struct fs_context *fc)
 	STEAL_STRING(cifs_sb, ctx, UNC);
 	STEAL_STRING(cifs_sb, ctx, source);
 	STEAL_STRING(cifs_sb, ctx, username);
+
 	if (need_recon == false)
 		STEAL_STRING_SENSITIVE(cifs_sb, ctx, password);
 	else  {
-		kfree_sensitive(ses->password);
-		ses->password = kstrdup(ctx->password, GFP_KERNEL);
-		if (!ses->password)
-			return -ENOMEM;
-		kfree_sensitive(ses->password2);
-		ses->password2 = kstrdup(ctx->password2, GFP_KERNEL);
-		if (!ses->password2) {
-			kfree_sensitive(ses->password);
-			ses->password = NULL;
+		if (ctx->password) {
+			new_password = kstrdup(ctx->password, GFP_KERNEL);
+			if (!new_password)
+				return -ENOMEM;
+		} else
+			STEAL_STRING_SENSITIVE(cifs_sb, ctx, password);
+	}
+
+	/*
+	 * if a new password2 has been specified, then reset it's value
+	 * inside the ses struct
+	 */
+	if (ctx->password2) {
+		new_password2 = kstrdup(ctx->password2, GFP_KERNEL);
+		if (!new_password2) {
+			if (new_password)
+				kfree_sensitive(new_password);
 			return -ENOMEM;
 		}
+	} else
+		STEAL_STRING_SENSITIVE(cifs_sb, ctx, password2);
+
+	/*
+	 * we may update the passwords in the ses struct below. Make sure we do
+	 * not race with smb2_reconnect
+	 */
+	mutex_lock(&ses->session_mutex);
+
+	/*
+	 * smb2_reconnect may swap password and password2 in case session setup
+	 * failed. First get ctx passwords in sync with ses passwords. It should
+	 * be okay to do this even if this function were to return an error at a
+	 * later stage
+	 */
+	if (ses->password &&
+	    cifs_sb->ctx->password &&
+	    strcmp(ses->password, cifs_sb->ctx->password)) {
+		kfree_sensitive(cifs_sb->ctx->password);
+		cifs_sb->ctx->password = kstrdup(ses->password, GFP_KERNEL);
+	}
+	if (ses->password2 &&
+	    cifs_sb->ctx->password2 &&
+	    strcmp(ses->password2, cifs_sb->ctx->password2)) {
+		kfree_sensitive(cifs_sb->ctx->password2);
+		cifs_sb->ctx->password2 = kstrdup(ses->password2, GFP_KERNEL);
+	}
+
+	/*
+	 * now that allocations for passwords are done, commit them
+	 */
+	if (new_password) {
+		kfree_sensitive(ses->password);
+		ses->password = new_password;
+	}
+	if (new_password2) {
+		kfree_sensitive(ses->password2);
+		ses->password2 = new_password2;
 	}
+
+	mutex_unlock(&ses->session_mutex);
+
 	STEAL_STRING(cifs_sb, ctx, domainname);
 	STEAL_STRING(cifs_sb, ctx, nodename);
 	STEAL_STRING(cifs_sb, ctx, iocharset);
-- 
2.46.0.46.g406f326d27

[PATCH 2/2] cifs: support mounting with alternate password to allow password rotation

[meetakshisetiyaoss]

From: Meetakshi Setiya <msetiya@microsoft.com>

This patch introduces the following changes to support password rotation on
mount:

1. If an existing session is not found and the new session setup results in
EACCES, EKEYEXPIRED or EKEYREVOKED, swap password and password2 (if
available), and retry the mount.

2. To match the new mount with an existing session, add conditions to check
if a) password and password2 of the new mount and the existing session are
the same, or b) password of the new mount is the same as the password2 of
the existing session, and password2 of the new mount is the same as the
password of the existing session.

3. If an existing session is found, but needs reconnect, retry the session
setup after swapping password and password2 (if available), in case the
previous attempt results in EACCES, EKEYEXPIRED or EKEYREVOKED.

Signed-off-by: Meetakshi Setiya <msetiya@microsoft.com>
---
 fs/smb/client/connect.c | 57 ++++++++++++++++++++++++++++++++++++-----
 1 file changed, 50 insertions(+), 7 deletions(-)

diff --git a/fs/smb/client/connect.c b/fs/smb/client/connect.c
index 15d94ac4095e..71b305230e14 100644
--- a/fs/smb/client/connect.c
+++ b/fs/smb/client/connect.c
@@ -1898,11 +1898,35 @@ static int match_session(struct cifs_ses *ses,
 			    CIFS_MAX_USERNAME_LEN))
 			return 0;
 		if ((ctx->username && strlen(ctx->username) != 0) &&
-		    ses->password != NULL &&
-		    strncmp(ses->password,
-			    ctx->password ? ctx->password : "",
-			    CIFS_MAX_PASSWORD_LEN))
-			return 0;
+		    ses->password != NULL) {
+
+			/* New mount can only share sessions with an existing mount if:
+			 * 1. Both password and password2 match, or
+			 * 2. password2 of the old mount matches password of the new mount
+			 *    and password of the old mount matches password2 of the new
+			 *	  mount
+			 */
+			if (ses->password2 != NULL && ctx->password2 != NULL) {
+				if (!((strncmp(ses->password, ctx->password ?
+					ctx->password : "", CIFS_MAX_PASSWORD_LEN) == 0 &&
+					strncmp(ses->password2, ctx->password2,
+					CIFS_MAX_PASSWORD_LEN) == 0) ||
+					(strncmp(ses->password, ctx->password2,
+					CIFS_MAX_PASSWORD_LEN) == 0 &&
+					strncmp(ses->password2, ctx->password ?
+					ctx->password : "", CIFS_MAX_PASSWORD_LEN) == 0)))
+					return 0;
+
+			} else if ((ses->password2 == NULL && ctx->password2 != NULL) ||
+				(ses->password2 != NULL && ctx->password2 == NULL)) {
+				return 0;
+
+			} else {
+				if (strncmp(ses->password, ctx->password ?
+					ctx->password : "", CIFS_MAX_PASSWORD_LEN))
+					return 0;
+			}
+		}
 	}
 
 	if (strcmp(ctx->local_nls->charset, ses->local_nls->charset))
@@ -2245,6 +2269,7 @@ struct cifs_ses *
 cifs_get_smb_ses(struct TCP_Server_Info *server, struct smb3_fs_context *ctx)
 {
 	int rc = 0;
+	int retries = 0;
 	unsigned int xid;
 	struct cifs_ses *ses;
 	struct sockaddr_in *addr = (struct sockaddr_in *)&server->dstaddr;
@@ -2263,6 +2288,8 @@ cifs_get_smb_ses(struct TCP_Server_Info *server, struct smb3_fs_context *ctx)
 			cifs_dbg(FYI, "Session needs reconnect\n");
 
 			mutex_lock(&ses->session_mutex);
+
+retry_old_session:
 			rc = cifs_negotiate_protocol(xid, ses, server);
 			if (rc) {
 				mutex_unlock(&ses->session_mutex);
@@ -2275,6 +2302,13 @@ cifs_get_smb_ses(struct TCP_Server_Info *server, struct smb3_fs_context *ctx)
 			rc = cifs_setup_session(xid, ses, server,
 						ctx->local_nls);
 			if (rc) {
+				if (((rc == -EACCES) || (rc == -EKEYEXPIRED) ||
+					(rc == -EKEYREVOKED)) && !retries && ses->password2) {
+					retries++;
+					cifs_info("Session reconnect failed, retrying with alternate password\n");
+					swap(ses->password, ses->password2);
+					goto retry_old_session;
+				}
 				mutex_unlock(&ses->session_mutex);
 				/* problem -- put our reference */
 				cifs_put_smb_ses(ses);
@@ -2350,6 +2384,7 @@ cifs_get_smb_ses(struct TCP_Server_Info *server, struct smb3_fs_context *ctx)
 	ses->chans_need_reconnect = 1;
 	spin_unlock(&ses->chan_lock);
 
+retry_new_session:
 	mutex_lock(&ses->session_mutex);
 	rc = cifs_negotiate_protocol(xid, ses, server);
 	if (!rc)
@@ -2362,8 +2397,16 @@ cifs_get_smb_ses(struct TCP_Server_Info *server, struct smb3_fs_context *ctx)
 	       sizeof(ses->smb3signingkey));
 	spin_unlock(&ses->chan_lock);
 
-	if (rc)
-		goto get_ses_fail;
+	if (rc) {
+		if (((rc == -EACCES) || (rc == -EKEYEXPIRED) ||
+			(rc == -EKEYREVOKED)) && !retries && ses->password2) {
+			retries++;
+			cifs_info("Session setup failed, retrying with alternate password\n");
+			swap(ses->password, ses->password2);
+			goto retry_new_session;
+		} else
+			goto get_ses_fail;
+	}
 
 	/*
 	 * success, put it on the list and add it as first channel
-- 
2.46.0.46.g406f326d27

Re: [PATCH 1/2] cifs: during remount, make sure passwords are in sync

[Paulo Alcantara]

meetakshisetiyaoss@gmail.com writes:

> From: Shyam Prasad N <sprasad@microsoft.com>
>
> We recently introduced a password2 field in both ses and ctx structs.
> This was done so as to allow the client to rotate passwords for a mount
> without any downtime. However, when the client transparently handles
> password rotation, it can swap the values of the two password fields
> in the ses struct, but not in smb3_fs_context struct that hangs off
> cifs_sb. This can lead to a situation where a remount unintentionally
> overwrites a working password in the ses struct.

I don't see password rotation being handled for SMB1.  I mounted a share
with 'vers=1.0,password=foo,password2=bar' and didn't get any warnings
or errors about it being usupported.  I think users would like to have
that.

What about SMB sessions from cifs_tcon::dfs_ses_list?  I don't see their
password getting updated over remount.

If you don't plan to support any of the above, then don't allow users to
mount/remount when password rotation can't be handled.

> In order to fix this, we first get the passwords in ctx struct
> in-sync with ses struct, before replacing them with what the passwords
> that could be passed as a part of remount.
>
> Also, in order to avoid race condition between smb2_reconnect and
> smb3_reconfigure, we make sure to lock session_mutex before changing
> password and password2 fields of the ses structure.
>
> Fixes: 35f834265e0d ("smb3: fix broken reconnect when password changing on the server by allowing password rotation")
> Signed-off-by: Shyam Prasad N <sprasad@microsoft.com>
> Signed-off-by: Meetakshi Setiya <msetiya@microsoft.com>
> ---
>  fs/smb/client/fs_context.c | 69 +++++++++++++++++++++++++++++++++-----
>  1 file changed, 60 insertions(+), 9 deletions(-)
>
> diff --git a/fs/smb/client/fs_context.c b/fs/smb/client/fs_context.c
> index 5c5a52019efa..73610e66c8d9 100644
> --- a/fs/smb/client/fs_context.c
> +++ b/fs/smb/client/fs_context.c
> @@ -896,6 +896,7 @@ static int smb3_reconfigure(struct fs_context *fc)
>  	struct dentry *root = fc->root;
>  	struct cifs_sb_info *cifs_sb = CIFS_SB(root->d_sb);
>  	struct cifs_ses *ses = cifs_sb_master_tcon(cifs_sb)->ses;
> +	char *new_password = NULL, *new_password2 = NULL;
>  	bool need_recon = false;
>  	int rc;
>  
> @@ -915,21 +916,71 @@ static int smb3_reconfigure(struct fs_context *fc)
>  	STEAL_STRING(cifs_sb, ctx, UNC);
>  	STEAL_STRING(cifs_sb, ctx, source);
>  	STEAL_STRING(cifs_sb, ctx, username);
> +
>  	if (need_recon == false)
>  		STEAL_STRING_SENSITIVE(cifs_sb, ctx, password);
>  	else  {
> -		kfree_sensitive(ses->password);
> -		ses->password = kstrdup(ctx->password, GFP_KERNEL);
> -		if (!ses->password)
> -			return -ENOMEM;
> -		kfree_sensitive(ses->password2);
> -		ses->password2 = kstrdup(ctx->password2, GFP_KERNEL);
> -		if (!ses->password2) {
> -			kfree_sensitive(ses->password);
> -			ses->password = NULL;
> +		if (ctx->password) {
> +			new_password = kstrdup(ctx->password, GFP_KERNEL);
> +			if (!new_password)
> +				return -ENOMEM;
> +		} else
> +			STEAL_STRING_SENSITIVE(cifs_sb, ctx, password);
> +	}
> +
> +	/*
> +	 * if a new password2 has been specified, then reset it's value
> +	 * inside the ses struct
> +	 */
> +	if (ctx->password2) {
> +		new_password2 = kstrdup(ctx->password2, GFP_KERNEL);
> +		if (!new_password2) {
> +			if (new_password)

Useless non-NULL check as kfree_sensitive() already handles it.

> +				kfree_sensitive(new_password);
>  			return -ENOMEM;
>  		}
> +	} else
> +		STEAL_STRING_SENSITIVE(cifs_sb, ctx, password2);
> +
> +	/*
> +	 * we may update the passwords in the ses struct below. Make sure we do
> +	 * not race with smb2_reconnect
> +	 */
> +	mutex_lock(&ses->session_mutex);
> +
> +	/*
> +	 * smb2_reconnect may swap password and password2 in case session setup
> +	 * failed. First get ctx passwords in sync with ses passwords. It should
> +	 * be okay to do this even if this function were to return an error at a
> +	 * later stage
> +	 */
> +	if (ses->password &&
> +	    cifs_sb->ctx->password &&
> +	    strcmp(ses->password, cifs_sb->ctx->password)) {
> +		kfree_sensitive(cifs_sb->ctx->password);
> +		cifs_sb->ctx->password = kstrdup(ses->password, GFP_KERNEL);

Missing allocation failure check.

> +	}
> +	if (ses->password2 &&
> +	    cifs_sb->ctx->password2 &&
> +	    strcmp(ses->password2, cifs_sb->ctx->password2)) {
> +		kfree_sensitive(cifs_sb->ctx->password2);
> +		cifs_sb->ctx->password2 = kstrdup(ses->password2, GFP_KERNEL);

Ditto.

> +	}
> +
> +	/*
> +	 * now that allocations for passwords are done, commit them
> +	 */
> +	if (new_password) {
> +		kfree_sensitive(ses->password);
> +		ses->password = new_password;
> +	}
> +	if (new_password2) {
> +		kfree_sensitive(ses->password2);
> +		ses->password2 = new_password2;
>  	}
> +
> +	mutex_unlock(&ses->session_mutex);
> +
>  	STEAL_STRING(cifs_sb, ctx, domainname);
>  	STEAL_STRING(cifs_sb, ctx, nodename);
>  	STEAL_STRING(cifs_sb, ctx, iocharset);
> -- 
> 2.46.0.46.g406f326d27

Re: [PATCH 2/2] cifs: support mounting with alternate password to allow password rotation

[Paulo Alcantara]

meetakshisetiyaoss@gmail.com writes:

> @@ -2245,6 +2269,7 @@ struct cifs_ses *
>  cifs_get_smb_ses(struct TCP_Server_Info *server, struct smb3_fs_context *ctx)
>  {
>  	int rc = 0;
> +	int retries = 0;
>  	unsigned int xid;
>  	struct cifs_ses *ses;
>  	struct sockaddr_in *addr = (struct sockaddr_in *)&server->dstaddr;
> @@ -2263,6 +2288,8 @@ cifs_get_smb_ses(struct TCP_Server_Info *server, struct smb3_fs_context *ctx)
>  			cifs_dbg(FYI, "Session needs reconnect\n");
>  
>  			mutex_lock(&ses->session_mutex);
> +
> +retry_old_session:
>  			rc = cifs_negotiate_protocol(xid, ses, server);
>  			if (rc) {
>  				mutex_unlock(&ses->session_mutex);
> @@ -2275,6 +2302,13 @@ cifs_get_smb_ses(struct TCP_Server_Info *server, struct smb3_fs_context *ctx)
>  			rc = cifs_setup_session(xid, ses, server,
>  						ctx->local_nls);
>  			if (rc) {
> +				if (((rc == -EACCES) || (rc == -EKEYEXPIRED) ||
> +					(rc == -EKEYREVOKED)) && !retries && ses->password2) {
> +					retries++;
> +					cifs_info("Session reconnect failed, retrying with alternate password\n");

Please don't add more noisy messages over reconnect.  Remember that if
SMB session doesn't get re-established, there will be flood enough on
dmesg with "Send error in SessSetup = ..." messages on every 2s that
already pisses off users and customers.

> +					swap(ses->password, ses->password2);
> +					goto retry_old_session;
> +				}
>  				mutex_unlock(&ses->session_mutex);
>  				/* problem -- put our reference */
>  				cifs_put_smb_ses(ses);
> @@ -2350,6 +2384,7 @@ cifs_get_smb_ses(struct TCP_Server_Info *server, struct smb3_fs_context *ctx)
>  	ses->chans_need_reconnect = 1;
>  	spin_unlock(&ses->chan_lock);
>  
> +retry_new_session:
>  	mutex_lock(&ses->session_mutex);
>  	rc = cifs_negotiate_protocol(xid, ses, server);
>  	if (!rc)
> @@ -2362,8 +2397,16 @@ cifs_get_smb_ses(struct TCP_Server_Info *server, struct smb3_fs_context *ctx)
>  	       sizeof(ses->smb3signingkey));
>  	spin_unlock(&ses->chan_lock);
>  
> -	if (rc)
> -		goto get_ses_fail;
> +	if (rc) {
> +		if (((rc == -EACCES) || (rc == -EKEYEXPIRED) ||
> +			(rc == -EKEYREVOKED)) && !retries && ses->password2) {
> +			retries++;
> +			cifs_info("Session setup failed, retrying with alternate password\n");

Ditto.

> +			swap(ses->password, ses->password2);
> +			goto retry_new_session;
> +		} else
> +			goto get_ses_fail;
> +	}
>  
>  	/*
>  	 * success, put it on the list and add it as first channel
> -- 
> 2.46.0.46.g406f326d27

Re: [PATCH 2/2] cifs: support mounting with alternate password to allow password rotation

[Shyam Prasad N]

On Fri, Nov 8, 2024 at 12:35 AM Paulo Alcantara <pc@manguebit.com> wrote:
>
> meetakshisetiyaoss@gmail.com writes:
>
> > @@ -2245,6 +2269,7 @@ struct cifs_ses *
> >  cifs_get_smb_ses(struct TCP_Server_Info *server, struct smb3_fs_context *ctx)
> >  {
> >       int rc = 0;
> > +     int retries = 0;
> >       unsigned int xid;
> >       struct cifs_ses *ses;
> >       struct sockaddr_in *addr = (struct sockaddr_in *)&server->dstaddr;
> > @@ -2263,6 +2288,8 @@ cifs_get_smb_ses(struct TCP_Server_Info *server, struct smb3_fs_context *ctx)
> >                       cifs_dbg(FYI, "Session needs reconnect\n");
> >
> >                       mutex_lock(&ses->session_mutex);
> > +
> > +retry_old_session:
> >                       rc = cifs_negotiate_protocol(xid, ses, server);
> >                       if (rc) {
> >                               mutex_unlock(&ses->session_mutex);
> > @@ -2275,6 +2302,13 @@ cifs_get_smb_ses(struct TCP_Server_Info *server, struct smb3_fs_context *ctx)
> >                       rc = cifs_setup_session(xid, ses, server,
> >                                               ctx->local_nls);
> >                       if (rc) {
> > +                             if (((rc == -EACCES) || (rc == -EKEYEXPIRED) ||
> > +                                     (rc == -EKEYREVOKED)) && !retries && ses->password2) {
> > +                                     retries++;
> > +                                     cifs_info("Session reconnect failed, retrying with alternate password\n");
>
> Please don't add more noisy messages over reconnect.  Remember that if
> SMB session doesn't get re-established, there will be flood enough on
> dmesg with "Send error in SessSetup = ..." messages on every 2s that
> already pisses off users and customers.
>
Perhaps we could do a cifs_dbg instead of cifs_info.
But Paulo, the problem here is that we retry every 2s. I think we
should address that instead.
One way is to do an exponential backoff every time we retry.

I'd also want to understand why we need the reconnect work? Why not
always do smb2_reconnect when someone does filesystem calls on the
mount point?
That way, we avoid unnecessary retries altogether. @Steve French your opinions?

> > +                                     swap(ses->password, ses->password2);
> > +                                     goto retry_old_session;
> > +                             }
> >                               mutex_unlock(&ses->session_mutex);
> >                               /* problem -- put our reference */
> >                               cifs_put_smb_ses(ses);
> > @@ -2350,6 +2384,7 @@ cifs_get_smb_ses(struct TCP_Server_Info *server, struct smb3_fs_context *ctx)
> >       ses->chans_need_reconnect = 1;
> >       spin_unlock(&ses->chan_lock);
> >
> > +retry_new_session:
> >       mutex_lock(&ses->session_mutex);
> >       rc = cifs_negotiate_protocol(xid, ses, server);
> >       if (!rc)
> > @@ -2362,8 +2397,16 @@ cifs_get_smb_ses(struct TCP_Server_Info *server, struct smb3_fs_context *ctx)
> >              sizeof(ses->smb3signingkey));
> >       spin_unlock(&ses->chan_lock);
> >
> > -     if (rc)
> > -             goto get_ses_fail;
> > +     if (rc) {
> > +             if (((rc == -EACCES) || (rc == -EKEYEXPIRED) ||
> > +                     (rc == -EKEYREVOKED)) && !retries && ses->password2) {
> > +                     retries++;
> > +                     cifs_info("Session setup failed, retrying with alternate password\n");
>
> Ditto.
>
> > +                     swap(ses->password, ses->password2);
> > +                     goto retry_new_session;
> > +             } else
> > +                     goto get_ses_fail;
> > +     }
> >
> >       /*
> >        * success, put it on the list and add it as first channel
> > --
> > 2.46.0.46.g406f326d27



-- 
Regards,
Shyam

Re: [PATCH 1/2] cifs: during remount, make sure passwords are in sync

[Shyam Prasad N]

Hi Paulo,

Thanks for the review.

On Fri, Nov 8, 2024 at 12:01 AM Paulo Alcantara <pc@manguebit.com> wrote:
>
> meetakshisetiyaoss@gmail.com writes:
>
> > From: Shyam Prasad N <sprasad@microsoft.com>
> >
> > We recently introduced a password2 field in both ses and ctx structs.
> > This was done so as to allow the client to rotate passwords for a mount
> > without any downtime. However, when the client transparently handles
> > password rotation, it can swap the values of the two password fields
> > in the ses struct, but not in smb3_fs_context struct that hangs off
> > cifs_sb. This can lead to a situation where a remount unintentionally
> > overwrites a working password in the ses struct.
>
> I don't see password rotation being handled for SMB1.  I mounted a share
> with 'vers=1.0,password=foo,password2=bar' and didn't get any warnings
> or errors about it being usupported.  I think users would like to have
> that.

Good point. We could add support for SMB1 or document clearly that
this is only for SMB2+.

>
> What about SMB sessions from cifs_tcon::dfs_ses_list?  I don't see their
> password getting updated over remount.

This is in our to-do list as well.

>
> If you don't plan to support any of the above, then don't allow users to
> mount/remount when password rotation can't be handled.
>
> > In order to fix this, we first get the passwords in ctx struct
> > in-sync with ses struct, before replacing them with what the passwords
> > that could be passed as a part of remount.
> >
> > Also, in order to avoid race condition between smb2_reconnect and
> > smb3_reconfigure, we make sure to lock session_mutex before changing
> > password and password2 fields of the ses structure.
> >
> > Fixes: 35f834265e0d ("smb3: fix broken reconnect when password changing on the server by allowing password rotation")
> > Signed-off-by: Shyam Prasad N <sprasad@microsoft.com>
> > Signed-off-by: Meetakshi Setiya <msetiya@microsoft.com>
> > ---
> >  fs/smb/client/fs_context.c | 69 +++++++++++++++++++++++++++++++++-----
> >  1 file changed, 60 insertions(+), 9 deletions(-)
> >
> > diff --git a/fs/smb/client/fs_context.c b/fs/smb/client/fs_context.c
> > index 5c5a52019efa..73610e66c8d9 100644
> > --- a/fs/smb/client/fs_context.c
> > +++ b/fs/smb/client/fs_context.c
> > @@ -896,6 +896,7 @@ static int smb3_reconfigure(struct fs_context *fc)
> >       struct dentry *root = fc->root;
> >       struct cifs_sb_info *cifs_sb = CIFS_SB(root->d_sb);
> >       struct cifs_ses *ses = cifs_sb_master_tcon(cifs_sb)->ses;
> > +     char *new_password = NULL, *new_password2 = NULL;
> >       bool need_recon = false;
> >       int rc;
> >
> > @@ -915,21 +916,71 @@ static int smb3_reconfigure(struct fs_context *fc)
> >       STEAL_STRING(cifs_sb, ctx, UNC);
> >       STEAL_STRING(cifs_sb, ctx, source);
> >       STEAL_STRING(cifs_sb, ctx, username);
> > +
> >       if (need_recon == false)
> >               STEAL_STRING_SENSITIVE(cifs_sb, ctx, password);
> >       else  {
> > -             kfree_sensitive(ses->password);
> > -             ses->password = kstrdup(ctx->password, GFP_KERNEL);
> > -             if (!ses->password)
> > -                     return -ENOMEM;
> > -             kfree_sensitive(ses->password2);
> > -             ses->password2 = kstrdup(ctx->password2, GFP_KERNEL);
> > -             if (!ses->password2) {
> > -                     kfree_sensitive(ses->password);
> > -                     ses->password = NULL;
> > +             if (ctx->password) {
> > +                     new_password = kstrdup(ctx->password, GFP_KERNEL);
> > +                     if (!new_password)
> > +                             return -ENOMEM;
> > +             } else
> > +                     STEAL_STRING_SENSITIVE(cifs_sb, ctx, password);
> > +     }
> > +
> > +     /*
> > +      * if a new password2 has been specified, then reset it's value
> > +      * inside the ses struct
> > +      */
> > +     if (ctx->password2) {
> > +             new_password2 = kstrdup(ctx->password2, GFP_KERNEL);
> > +             if (!new_password2) {
> > +                     if (new_password)
>
> Useless non-NULL check as kfree_sensitive() already handles it.
>
> > +                             kfree_sensitive(new_password);
> >                       return -ENOMEM;
> >               }
> > +     } else
> > +             STEAL_STRING_SENSITIVE(cifs_sb, ctx, password2);
> > +
> > +     /*
> > +      * we may update the passwords in the ses struct below. Make sure we do
> > +      * not race with smb2_reconnect
> > +      */
> > +     mutex_lock(&ses->session_mutex);
> > +
> > +     /*
> > +      * smb2_reconnect may swap password and password2 in case session setup
> > +      * failed. First get ctx passwords in sync with ses passwords. It should
> > +      * be okay to do this even if this function were to return an error at a
> > +      * later stage
> > +      */
> > +     if (ses->password &&
> > +         cifs_sb->ctx->password &&
> > +         strcmp(ses->password, cifs_sb->ctx->password)) {
> > +             kfree_sensitive(cifs_sb->ctx->password);
> > +             cifs_sb->ctx->password = kstrdup(ses->password, GFP_KERNEL);
>
> Missing allocation failure check.
>
> > +     }
> > +     if (ses->password2 &&
> > +         cifs_sb->ctx->password2 &&
> > +         strcmp(ses->password2, cifs_sb->ctx->password2)) {
> > +             kfree_sensitive(cifs_sb->ctx->password2);
> > +             cifs_sb->ctx->password2 = kstrdup(ses->password2, GFP_KERNEL);
>
> Ditto.
>
> > +     }
> > +
> > +     /*
> > +      * now that allocations for passwords are done, commit them
> > +      */
> > +     if (new_password) {
> > +             kfree_sensitive(ses->password);
> > +             ses->password = new_password;
> > +     }
> > +     if (new_password2) {
> > +             kfree_sensitive(ses->password2);
> > +             ses->password2 = new_password2;
> >       }
> > +
> > +     mutex_unlock(&ses->session_mutex);
> > +
> >       STEAL_STRING(cifs_sb, ctx, domainname);
> >       STEAL_STRING(cifs_sb, ctx, nodename);
> >       STEAL_STRING(cifs_sb, ctx, iocharset);
> > --
> > 2.46.0.46.g406f326d27



-- 
Regards,
Shyam

Re: [PATCH 2/2] cifs: support mounting with alternate password to allow password rotation

[Paulo Alcantara]

Shyam Prasad N <nspmangalore@gmail.com> writes:

> On Fri, Nov 8, 2024 at 12:35 AM Paulo Alcantara <pc@manguebit.com> wrote:
>>
>> meetakshisetiyaoss@gmail.com writes:
>>
>> > @@ -2245,6 +2269,7 @@ struct cifs_ses *
>> >  cifs_get_smb_ses(struct TCP_Server_Info *server, struct smb3_fs_context *ctx)
>> >  {
>> >       int rc = 0;
>> > +     int retries = 0;
>> >       unsigned int xid;
>> >       struct cifs_ses *ses;
>> >       struct sockaddr_in *addr = (struct sockaddr_in *)&server->dstaddr;
>> > @@ -2263,6 +2288,8 @@ cifs_get_smb_ses(struct TCP_Server_Info *server, struct smb3_fs_context *ctx)
>> >                       cifs_dbg(FYI, "Session needs reconnect\n");
>> >
>> >                       mutex_lock(&ses->session_mutex);
>> > +
>> > +retry_old_session:
>> >                       rc = cifs_negotiate_protocol(xid, ses, server);
>> >                       if (rc) {
>> >                               mutex_unlock(&ses->session_mutex);
>> > @@ -2275,6 +2302,13 @@ cifs_get_smb_ses(struct TCP_Server_Info *server, struct smb3_fs_context *ctx)
>> >                       rc = cifs_setup_session(xid, ses, server,
>> >                                               ctx->local_nls);
>> >                       if (rc) {
>> > +                             if (((rc == -EACCES) || (rc == -EKEYEXPIRED) ||
>> > +                                     (rc == -EKEYREVOKED)) && !retries && ses->password2) {
>> > +                                     retries++;
>> > +                                     cifs_info("Session reconnect failed, retrying with alternate password\n");
>>
>> Please don't add more noisy messages over reconnect.  Remember that if
>> SMB session doesn't get re-established, there will be flood enough on
>> dmesg with "Send error in SessSetup = ..." messages on every 2s that
>> already pisses off users and customers.
>>
> Perhaps we could do a cifs_dbg instead of cifs_info.

Yep, with FYI.

> But Paulo, the problem here is that we retry every 2s. I think we
> should address that instead.
> One way is to do an exponential backoff every time we retry.

Agreed, but that doesn't mean we should add more noisy messages.

> I'd also want to understand why we need the reconnect work?

I see it as an optimisation to allow next IOs to not take longer because
it needs to reconnect SMB session.  If there was no prior filesystem
activity, why don't allow the client itself to reconnect the session?

Besides, SMB2_IOCTL currently doesn't call smb2_reconnect(), so
reconnect worker would be required to reconnect the session and then
allow SMB2_IOCTL to work.  We'd need to change that because with recent
special file support, we need to avoid failures when creating or parsing
reparse points because the SMB session isn't re-established yet.

> Why not always do smb2_reconnect when someone does filesystem calls on
> the mount point?

We already do for most operations.  SMB2_IOCTL and SMB2_TREE_CONNECT,
for instance, can't call smb2_reconnect() as they would deadlock.

Re: [PATCH 1/2] cifs: during remount, make sure passwords are in sync

[Shyam Prasad N]

On Fri, Nov 8, 2024 at 5:47 PM Shyam Prasad N <nspmangalore@gmail.com> wrote:
>
> Hi Paulo,
>
> Thanks for the review.
>
> On Fri, Nov 8, 2024 at 12:01 AM Paulo Alcantara <pc@manguebit.com> wrote:
> >
> > meetakshisetiyaoss@gmail.com writes:
> >
> > > From: Shyam Prasad N <sprasad@microsoft.com>
> > >
> > > We recently introduced a password2 field in both ses and ctx structs.
> > > This was done so as to allow the client to rotate passwords for a mount
> > > without any downtime. However, when the client transparently handles
> > > password rotation, it can swap the values of the two password fields
> > > in the ses struct, but not in smb3_fs_context struct that hangs off
> > > cifs_sb. This can lead to a situation where a remount unintentionally
> > > overwrites a working password in the ses struct.
> >
> > I don't see password rotation being handled for SMB1.  I mounted a share
> > with 'vers=1.0,password=foo,password2=bar' and didn't get any warnings
> > or errors about it being usupported.  I think users would like to have
> > that.
>
> Good point. We could add support for SMB1 or document clearly that
> this is only for SMB2+.
>
> >
> > What about SMB sessions from cifs_tcon::dfs_ses_list?  I don't see their
> > password getting updated over remount.
>
> This is in our to-do list as well.

I did some code reading around how DFS automount works.
@Paulo Alcantara Correct me if I'm wrong, but it sounds like we make
an assumption that when a DFS namespace has a junction to another
share, the same credentials are to be used to perform the mount of
that share. Is that always the case?
If we go by that assumption, for password2 to work with DFS mounts, we
only need to make sure that in cifs_do_automount, cur_ctx passwords
are synced up to the current ses passwords. That should be quite easy.

>
> >
> > If you don't plan to support any of the above, then don't allow users to
> > mount/remount when password rotation can't be handled.
> >
> > > In order to fix this, we first get the passwords in ctx struct
> > > in-sync with ses struct, before replacing them with what the passwords
> > > that could be passed as a part of remount.
> > >
> > > Also, in order to avoid race condition between smb2_reconnect and
> > > smb3_reconfigure, we make sure to lock session_mutex before changing
> > > password and password2 fields of the ses structure.
> > >
> > > Fixes: 35f834265e0d ("smb3: fix broken reconnect when password changing on the server by allowing password rotation")
> > > Signed-off-by: Shyam Prasad N <sprasad@microsoft.com>
> > > Signed-off-by: Meetakshi Setiya <msetiya@microsoft.com>
> > > ---
> > >  fs/smb/client/fs_context.c | 69 +++++++++++++++++++++++++++++++++-----
> > >  1 file changed, 60 insertions(+), 9 deletions(-)
> > >
> > > diff --git a/fs/smb/client/fs_context.c b/fs/smb/client/fs_context.c
> > > index 5c5a52019efa..73610e66c8d9 100644
> > > --- a/fs/smb/client/fs_context.c
> > > +++ b/fs/smb/client/fs_context.c
> > > @@ -896,6 +896,7 @@ static int smb3_reconfigure(struct fs_context *fc)
> > >       struct dentry *root = fc->root;
> > >       struct cifs_sb_info *cifs_sb = CIFS_SB(root->d_sb);
> > >       struct cifs_ses *ses = cifs_sb_master_tcon(cifs_sb)->ses;
> > > +     char *new_password = NULL, *new_password2 = NULL;
> > >       bool need_recon = false;
> > >       int rc;
> > >
> > > @@ -915,21 +916,71 @@ static int smb3_reconfigure(struct fs_context *fc)
> > >       STEAL_STRING(cifs_sb, ctx, UNC);
> > >       STEAL_STRING(cifs_sb, ctx, source);
> > >       STEAL_STRING(cifs_sb, ctx, username);
> > > +
> > >       if (need_recon == false)
> > >               STEAL_STRING_SENSITIVE(cifs_sb, ctx, password);
> > >       else  {
> > > -             kfree_sensitive(ses->password);
> > > -             ses->password = kstrdup(ctx->password, GFP_KERNEL);
> > > -             if (!ses->password)
> > > -                     return -ENOMEM;
> > > -             kfree_sensitive(ses->password2);
> > > -             ses->password2 = kstrdup(ctx->password2, GFP_KERNEL);
> > > -             if (!ses->password2) {
> > > -                     kfree_sensitive(ses->password);
> > > -                     ses->password = NULL;
> > > +             if (ctx->password) {
> > > +                     new_password = kstrdup(ctx->password, GFP_KERNEL);
> > > +                     if (!new_password)
> > > +                             return -ENOMEM;
> > > +             } else
> > > +                     STEAL_STRING_SENSITIVE(cifs_sb, ctx, password);
> > > +     }
> > > +
> > > +     /*
> > > +      * if a new password2 has been specified, then reset it's value
> > > +      * inside the ses struct
> > > +      */
> > > +     if (ctx->password2) {
> > > +             new_password2 = kstrdup(ctx->password2, GFP_KERNEL);
> > > +             if (!new_password2) {
> > > +                     if (new_password)
> >
> > Useless non-NULL check as kfree_sensitive() already handles it.
> >
> > > +                             kfree_sensitive(new_password);
> > >                       return -ENOMEM;
> > >               }
> > > +     } else
> > > +             STEAL_STRING_SENSITIVE(cifs_sb, ctx, password2);
> > > +
> > > +     /*
> > > +      * we may update the passwords in the ses struct below. Make sure we do
> > > +      * not race with smb2_reconnect
> > > +      */
> > > +     mutex_lock(&ses->session_mutex);
> > > +
> > > +     /*
> > > +      * smb2_reconnect may swap password and password2 in case session setup
> > > +      * failed. First get ctx passwords in sync with ses passwords. It should
> > > +      * be okay to do this even if this function were to return an error at a
> > > +      * later stage
> > > +      */
> > > +     if (ses->password &&
> > > +         cifs_sb->ctx->password &&
> > > +         strcmp(ses->password, cifs_sb->ctx->password)) {
> > > +             kfree_sensitive(cifs_sb->ctx->password);
> > > +             cifs_sb->ctx->password = kstrdup(ses->password, GFP_KERNEL);
> >
> > Missing allocation failure check.
> >
> > > +     }
> > > +     if (ses->password2 &&
> > > +         cifs_sb->ctx->password2 &&
> > > +         strcmp(ses->password2, cifs_sb->ctx->password2)) {
> > > +             kfree_sensitive(cifs_sb->ctx->password2);
> > > +             cifs_sb->ctx->password2 = kstrdup(ses->password2, GFP_KERNEL);
> >
> > Ditto.
> >
> > > +     }
> > > +
> > > +     /*
> > > +      * now that allocations for passwords are done, commit them
> > > +      */
> > > +     if (new_password) {
> > > +             kfree_sensitive(ses->password);
> > > +             ses->password = new_password;
> > > +     }
> > > +     if (new_password2) {
> > > +             kfree_sensitive(ses->password2);
> > > +             ses->password2 = new_password2;
> > >       }
> > > +
> > > +     mutex_unlock(&ses->session_mutex);
> > > +
> > >       STEAL_STRING(cifs_sb, ctx, domainname);
> > >       STEAL_STRING(cifs_sb, ctx, nodename);
> > >       STEAL_STRING(cifs_sb, ctx, iocharset);
> > > --
> > > 2.46.0.46.g406f326d27
>
>
>
> --
> Regards,
> Shyam



-- 
Regards,
Shyam

Re: [PATCH 1/2] cifs: during remount, make sure passwords are in sync

[Paulo Alcantara]

Shyam Prasad N <nspmangalore@gmail.com> writes:

> On Fri, Nov 8, 2024 at 5:47 PM Shyam Prasad N <nspmangalore@gmail.com> wrote:
>> > What about SMB sessions from cifs_tcon::dfs_ses_list?  I don't see their
>> > password getting updated over remount.
>>
>> This is in our to-do list as well.
>
> I did some code reading around how DFS automount works.
> @Paulo Alcantara Correct me if I'm wrong, but it sounds like we make
> an assumption that when a DFS namespace has a junction to another
> share, the same credentials are to be used to perform the mount of
> that share. Is that always the case?

Yes, it inherits fs_context from the parent mount.  For multiuser
mounts, when uid/gid/cruid are unspecified, we need to update its values
to match real uid/gid from the calling process.

> If we go by that assumption, for password2 to work with DFS mounts, we
> only need to make sure that in cifs_do_automount, cur_ctx passwords
> are synced up to the current ses passwords. That should be quite easy.

Correct.  The fs_context for the automount is dup'ed from the parent
mount.  smb3_fs_context_dup() already dups password2, so it should work.

The 'remount' case isn't still handled, that's why I mentioned it above.
You'd need to set password2 for all sessios in @tcon->dfs_ses_list.

I think we need to update password2 for the multiuser sessions as well
and not only for session from master tcon.

Re: [PATCH 2/2] cifs: support mounting with alternate password to allow password rotation

[Meetakshi Setiya]

[-- Attachment #1: Type: text/plain, Size: 3394 bytes --]

Thanks for the review Paulo, here is the updated patch that uses
cifs_dbg (FYI) instead of cifs_info.

Best
Meetakshi

On Fri, Nov 8, 2024 at 8:50 PM Paulo Alcantara <pc@manguebit.com> wrote:
>
> Shyam Prasad N <nspmangalore@gmail.com> writes:
>
> > On Fri, Nov 8, 2024 at 12:35 AM Paulo Alcantara <pc@manguebit.com> wrote:
> >>
> >> meetakshisetiyaoss@gmail.com writes:
> >>
> >> > @@ -2245,6 +2269,7 @@ struct cifs_ses *
> >> >  cifs_get_smb_ses(struct TCP_Server_Info *server, struct smb3_fs_context *ctx)
> >> >  {
> >> >       int rc = 0;
> >> > +     int retries = 0;
> >> >       unsigned int xid;
> >> >       struct cifs_ses *ses;
> >> >       struct sockaddr_in *addr = (struct sockaddr_in *)&server->dstaddr;
> >> > @@ -2263,6 +2288,8 @@ cifs_get_smb_ses(struct TCP_Server_Info *server, struct smb3_fs_context *ctx)
> >> >                       cifs_dbg(FYI, "Session needs reconnect\n");
> >> >
> >> >                       mutex_lock(&ses->session_mutex);
> >> > +
> >> > +retry_old_session:
> >> >                       rc = cifs_negotiate_protocol(xid, ses, server);
> >> >                       if (rc) {
> >> >                               mutex_unlock(&ses->session_mutex);
> >> > @@ -2275,6 +2302,13 @@ cifs_get_smb_ses(struct TCP_Server_Info *server, struct smb3_fs_context *ctx)
> >> >                       rc = cifs_setup_session(xid, ses, server,
> >> >                                               ctx->local_nls);
> >> >                       if (rc) {
> >> > +                             if (((rc == -EACCES) || (rc == -EKEYEXPIRED) ||
> >> > +                                     (rc == -EKEYREVOKED)) && !retries && ses->password2) {
> >> > +                                     retries++;
> >> > +                                     cifs_info("Session reconnect failed, retrying with alternate password\n");
> >>
> >> Please don't add more noisy messages over reconnect.  Remember that if
> >> SMB session doesn't get re-established, there will be flood enough on
> >> dmesg with "Send error in SessSetup = ..." messages on every 2s that
> >> already pisses off users and customers.
> >>
> > Perhaps we could do a cifs_dbg instead of cifs_info.
>
> Yep, with FYI.
>
> > But Paulo, the problem here is that we retry every 2s. I think we
> > should address that instead.
> > One way is to do an exponential backoff every time we retry.
>
> Agreed, but that doesn't mean we should add more noisy messages.
>
> > I'd also want to understand why we need the reconnect work?
>
> I see it as an optimisation to allow next IOs to not take longer because
> it needs to reconnect SMB session.  If there was no prior filesystem
> activity, why don't allow the client itself to reconnect the session?
>
> Besides, SMB2_IOCTL currently doesn't call smb2_reconnect(), so
> reconnect worker would be required to reconnect the session and then
> allow SMB2_IOCTL to work.  We'd need to change that because with recent
> special file support, we need to avoid failures when creating or parsing
> reparse points because the SMB session isn't re-established yet.
>
> > Why not always do smb2_reconnect when someone does filesystem calls on
> > the mount point?
>
> We already do for most operations.  SMB2_IOCTL and SMB2_TREE_CONNECT,
> for instance, can't call smb2_reconnect() as they would deadlock.

[-- Attachment #2: 0002-cifs-support-mounting-with-alternate-password-to-all.patch --]
[-- Type: application/octet-stream, Size: 4930 bytes --]

From 32f6e293f8f49f66fa4df17c2aa81569c7d7752e Mon Sep 17 00:00:00 2001
From: Meetakshi Setiya <msetiya@microsoft.com>
Date: Wed, 30 Oct 2024 05:37:21 -0400
Subject: [PATCH 2/2] cifs: support mounting with alternate password to allow
 password rotation

This patch introduces the following changes to support password rotation on
mount:

1. If an existing session is not found and the new session setup results in
EACCES, EKEYEXPIRED or EKEYREVOKED, swap password and password2 (if
available), and retry the mount.

2. To match the new mount with an existing session, add conditions to check
if a) password and password2 of the new mount and the existing session are
the same, or b) password of the new mount is the same as the password2 of
the existing session, and password2 of the new mount is the same as the
password of the existing session.

3. If an existing session is found, but needs reconnect, retry the session
setup after swapping password and password2 (if available), in case the
previous attempt results in EACCES, EKEYEXPIRED or EKEYREVOKED.

Signed-off-by: Meetakshi Setiya <msetiya@microsoft.com>
---
 fs/smb/client/connect.c | 57 ++++++++++++++++++++++++++++++++++++-----
 1 file changed, 50 insertions(+), 7 deletions(-)

diff --git a/fs/smb/client/connect.c b/fs/smb/client/connect.c
index 15d94ac4095e..029ddb358be0 100644
--- a/fs/smb/client/connect.c
+++ b/fs/smb/client/connect.c
@@ -1898,11 +1898,35 @@ static int match_session(struct cifs_ses *ses,
 			    CIFS_MAX_USERNAME_LEN))
 			return 0;
 		if ((ctx->username && strlen(ctx->username) != 0) &&
-		    ses->password != NULL &&
-		    strncmp(ses->password,
-			    ctx->password ? ctx->password : "",
-			    CIFS_MAX_PASSWORD_LEN))
-			return 0;
+		    ses->password != NULL) {
+
+			/* New mount can only share sessions with an existing mount if:
+			 * 1. Both password and password2 match, or
+			 * 2. password2 of the old mount matches password of the new mount
+			 *    and password of the old mount matches password2 of the new
+			 *	  mount
+			 */
+			if (ses->password2 != NULL && ctx->password2 != NULL) {
+				if (!((strncmp(ses->password, ctx->password ?
+					ctx->password : "", CIFS_MAX_PASSWORD_LEN) == 0 &&
+					strncmp(ses->password2, ctx->password2,
+					CIFS_MAX_PASSWORD_LEN) == 0) ||
+					(strncmp(ses->password, ctx->password2,
+					CIFS_MAX_PASSWORD_LEN) == 0 &&
+					strncmp(ses->password2, ctx->password ?
+					ctx->password : "", CIFS_MAX_PASSWORD_LEN) == 0)))
+					return 0;
+
+			} else if ((ses->password2 == NULL && ctx->password2 != NULL) ||
+				(ses->password2 != NULL && ctx->password2 == NULL)) {
+				return 0;
+
+			} else {
+				if (strncmp(ses->password, ctx->password ?
+					ctx->password : "", CIFS_MAX_PASSWORD_LEN))
+					return 0;
+			}
+		}
 	}
 
 	if (strcmp(ctx->local_nls->charset, ses->local_nls->charset))
@@ -2245,6 +2269,7 @@ struct cifs_ses *
 cifs_get_smb_ses(struct TCP_Server_Info *server, struct smb3_fs_context *ctx)
 {
 	int rc = 0;
+	int retries = 0;
 	unsigned int xid;
 	struct cifs_ses *ses;
 	struct sockaddr_in *addr = (struct sockaddr_in *)&server->dstaddr;
@@ -2263,6 +2288,8 @@ cifs_get_smb_ses(struct TCP_Server_Info *server, struct smb3_fs_context *ctx)
 			cifs_dbg(FYI, "Session needs reconnect\n");
 
 			mutex_lock(&ses->session_mutex);
+
+retry_old_session:
 			rc = cifs_negotiate_protocol(xid, ses, server);
 			if (rc) {
 				mutex_unlock(&ses->session_mutex);
@@ -2275,6 +2302,13 @@ cifs_get_smb_ses(struct TCP_Server_Info *server, struct smb3_fs_context *ctx)
 			rc = cifs_setup_session(xid, ses, server,
 						ctx->local_nls);
 			if (rc) {
+				if (((rc == -EACCES) || (rc == -EKEYEXPIRED) ||
+					(rc == -EKEYREVOKED)) && !retries && ses->password2) {
+					retries++;
+					cifs_dbg(FYI, "Session reconnect failed, retrying with alternate password\n");
+					swap(ses->password, ses->password2);
+					goto retry_old_session;
+				}
 				mutex_unlock(&ses->session_mutex);
 				/* problem -- put our reference */
 				cifs_put_smb_ses(ses);
@@ -2350,6 +2384,7 @@ cifs_get_smb_ses(struct TCP_Server_Info *server, struct smb3_fs_context *ctx)
 	ses->chans_need_reconnect = 1;
 	spin_unlock(&ses->chan_lock);
 
+retry_new_session:
 	mutex_lock(&ses->session_mutex);
 	rc = cifs_negotiate_protocol(xid, ses, server);
 	if (!rc)
@@ -2362,8 +2397,16 @@ cifs_get_smb_ses(struct TCP_Server_Info *server, struct smb3_fs_context *ctx)
 	       sizeof(ses->smb3signingkey));
 	spin_unlock(&ses->chan_lock);
 
-	if (rc)
-		goto get_ses_fail;
+	if (rc) {
+		if (((rc == -EACCES) || (rc == -EKEYEXPIRED) ||
+			(rc == -EKEYREVOKED)) && !retries && ses->password2) {
+			retries++;
+			cifs_dbg(FYI, "Session setup failed, retrying with alternate password\n");
+			swap(ses->password, ses->password2);
+			goto retry_new_session;
+		} else
+			goto get_ses_fail;
+	}
 
 	/*
 	 * success, put it on the list and add it as first channel
-- 
2.46.0.46.g406f326d27

Re: [PATCH 1/2] cifs: during remount, make sure passwords are in sync

[Meetakshi Setiya]

[-- Attachment #1: Type: text/plain, Size: 3535 bytes --]

Thanks for the review, Paulo. I have attached the updated patch.
I will send the password rotation changes for DFS, SMB1 (and multiuser) later,
separately.

Best
Meetakshi

On Wed, Nov 13, 2024 at 3:30 PM Meetakshi Setiya
<meetakshisetiyaoss@gmail.com> wrote:
>
> Typo: password rotation for SMB1.0 is NOT supported for reconnects. It works for fresh
> mounts and remounts.
> Should I add support for reconnect or remove it completely?
>
> On Wed, Nov 13, 2024 at 3:20 PM Meetakshi Setiya <meetakshisetiyaoss@gmail.com> wrote:
>>
>> Hi Paulo,
>>
>> Given your and Shyam's comments, I am thinking of making the
>> following changes to the code to support password rotation for DFS:
>>
>> 1. For a fresh mount:
>>     - In cifs_do_automount, bring the passwords in fs_context in sync with
>>     the passwords in the session object before sending the context to the
>>     child/submount.
>>
>> 2. For a remount (of the root only):
>>     - In smb3_reconfigure, bring the passwords in the fs_context of the master
>>     tcon in sync with its session object passwords. After this is done, a new
>>     function will be called to iterate over the dfs_ses_list held in this tcon
>>     and sync their session passwords with the updated root session password
>>     and password2.
>>
>> Password rotation for multiuser mounts is out of scope for this patch and I will
>> address it later.
>>
>> Please let me know if you have any comments or suggestions on this approach.
>>
>> Also, we share the mount code path (cifs_mount) for all SMB versions. So, password
>> rotation for SMB1.0 is currently supported ONLY on mounts. Password rotation on
>> remount, however, is not. Should I remove the support completely for SMB1.0
>> (print a warning message), leave it be, or add remount support?
>
>
> reconnect, not remount.
>
>>
>> Thanks
>> Meetakshi
>>
>> On Mon, Nov 11, 2024 at 5:34 PM Paulo Alcantara <pc@manguebit.com> wrote:
>>>
>>> Shyam Prasad N <nspmangalore@gmail.com> writes:
>>>
>>> > On Fri, Nov 8, 2024 at 5:47 PM Shyam Prasad N <nspmangalore@gmail.com> wrote:
>>> >> > What about SMB sessions from cifs_tcon::dfs_ses_list?  I don't see their
>>> >> > password getting updated over remount.
>>> >>
>>> >> This is in our to-do list as well.
>>> >
>>> > I did some code reading around how DFS automount works.
>>> > @Paulo Alcantara Correct me if I'm wrong, but it sounds like we make
>>> > an assumption that when a DFS namespace has a junction to another
>>> > share, the same credentials are to be used to perform the mount of
>>> > that share. Is that always the case?
>>>
>>> Yes, it inherits fs_context from the parent mount.  For multiuser
>>> mounts, when uid/gid/cruid are unspecified, we need to update its values
>>> to match real uid/gid from the calling process.
>>>
>>> > If we go by that assumption, for password2 to work with DFS mounts, we
>>> > only need to make sure that in cifs_do_automount, cur_ctx passwords
>>> > are synced up to the current ses passwords. That should be quite easy.
>>>
>>> Correct.  The fs_context for the automount is dup'ed from the parent
>>> mount.  smb3_fs_context_dup() already dups password2, so it should work.
>>>
>>> The 'remount' case isn't still handled, that's why I mentioned it above.
>>> You'd need to set password2 for all sessios in @tcon->dfs_ses_list.
>>>
>>> I think we need to update password2 for the multiuser sessions as well
>>> and not only for session from master tcon.

[-- Attachment #2: 0001-cifs-during-remount-make-sure-passwords-are-in-sync.patch --]
[-- Type: application/octet-stream, Size: 5414 bytes --]

From 25f8bd5f7447c2594b32024cbe16e211d1db1dbb Mon Sep 17 00:00:00 2001
From: Shyam Prasad N <sprasad@microsoft.com>
Date: Wed, 30 Oct 2024 06:45:50 +0000
Subject: [PATCH 1/2] cifs: during remount, make sure passwords are in sync

We recently introduced a password2 field in both ses and ctx structs.
This was done so as to allow the client to rotate passwords for a mount
without any downtime. However, when the client transparently handles
password rotation, it can swap the values of the two password fields
in the ses struct, but not in smb3_fs_context struct that hangs off
cifs_sb. This can lead to a situation where a remount unintentionally
overwrites a working password in the ses struct.

In order to fix this, we first get the passwords in ctx struct
in-sync with ses struct, before replacing them with what the passwords
that could be passed as a part of remount.

Also, in order to avoid race condition between smb2_reconnect and
smb3_reconfigure, we make sure to lock session_mutex before changing
password and password2 fields of the ses structure.

Fixes: 35f834265e0d ("smb3: fix broken reconnect when password changing on the server by allowing password rotation")
Signed-off-by: Shyam Prasad N <sprasad@microsoft.com>
Signed-off-by: Meetakshi Setiya <msetiya@microsoft.com>
---
 fs/smb/client/fs_context.c | 83 +++++++++++++++++++++++++++++++++-----
 fs/smb/client/fs_context.h |  1 +
 2 files changed, 75 insertions(+), 9 deletions(-)

diff --git a/fs/smb/client/fs_context.c b/fs/smb/client/fs_context.c
index 5c5a52019efa..bc68e6ebb99b 100644
--- a/fs/smb/client/fs_context.c
+++ b/fs/smb/client/fs_context.c
@@ -890,12 +890,37 @@ do {									\
 	cifs_sb->ctx->field = NULL;					\
 } while (0)
 
+int smb_sync_session_ctx_passwords(struct cifs_sb_info cifs_sb, struct cifs_ses ses)
+{
+	if (ses->password &&
+	    cifs_sb->ctx->password &&
+	    strcmp(ses->password, cifs_sb->ctx->password)) {
+		kfree_sensitive(cifs_sb->ctx->password);
+		cifs_sb->ctx->password = kstrdup(ses->password, GFP_KERNEL);
+		if (!cifs_sb->ctx->password)
+			return -ENOMEM;
+	}
+	if (ses->password2 &&
+	    cifs_sb->ctx->password2 &&
+	    strcmp(ses->password2, cifs_sb->ctx->password2)) {
+		kfree_sensitive(cifs_sb->ctx->password2);
+		cifs_sb->ctx->password2 = kstrdup(ses->password2, GFP_KERNEL);
+		if (!cifs_sb->ctx->password2) {
+			kfree_sensitive(cifs_sb->ctx->password);
+			cifs_sb->ctx->password = NULL;
+			return -ENOMEM;
+		}
+	}
+	return 0;
+}
+
 static int smb3_reconfigure(struct fs_context *fc)
 {
 	struct smb3_fs_context *ctx = smb3_fc2context(fc);
 	struct dentry *root = fc->root;
 	struct cifs_sb_info *cifs_sb = CIFS_SB(root->d_sb);
 	struct cifs_ses *ses = cifs_sb_master_tcon(cifs_sb)->ses;
+	char *new_password = NULL, *new_password2 = NULL;
 	bool need_recon = false;
 	int rc;
 
@@ -915,21 +940,61 @@ static int smb3_reconfigure(struct fs_context *fc)
 	STEAL_STRING(cifs_sb, ctx, UNC);
 	STEAL_STRING(cifs_sb, ctx, source);
 	STEAL_STRING(cifs_sb, ctx, username);
+
 	if (need_recon == false)
 		STEAL_STRING_SENSITIVE(cifs_sb, ctx, password);
 	else  {
-		kfree_sensitive(ses->password);
-		ses->password = kstrdup(ctx->password, GFP_KERNEL);
-		if (!ses->password)
-			return -ENOMEM;
-		kfree_sensitive(ses->password2);
-		ses->password2 = kstrdup(ctx->password2, GFP_KERNEL);
-		if (!ses->password2) {
-			kfree_sensitive(ses->password);
-			ses->password = NULL;
+		if (ctx->password) {
+			new_password = kstrdup(ctx->password, GFP_KERNEL);
+			if (!new_password)
+				return -ENOMEM;
+		} else
+			STEAL_STRING_SENSITIVE(cifs_sb, ctx, password);
+	}
+
+	/*
+	 * if a new password2 has been specified, then reset it's value
+	 * inside the ses struct
+	 */
+	if (ctx->password2) {
+		new_password2 = kstrdup(ctx->password2, GFP_KERNEL);
+		if (!new_password2) {
+			kfree_sensitive(new_password);
 			return -ENOMEM;
 		}
+	} else
+		STEAL_STRING_SENSITIVE(cifs_sb, ctx, password2);
+
+	/*
+	 * we may update the passwords in the ses struct below. Make sure we do
+	 * not race with smb2_reconnect
+	 */
+	mutex_lock(&ses->session_mutex);
+
+	/*
+	 * smb2_reconnect may swap password and password2 in case session setup
+	 * failed. First get ctx passwords in sync with ses passwords. It should
+	 * be okay to do this even if this function were to return an error at a
+	 * later stage
+	 */
+	rc = smb_sync_session_ctx_passwords(cifs_sb, ses);
+	if (rc)
+		return rc;
+
+	/*
+	 * now that allocations for passwords are done, commit them
+	 */
+	if (new_password) {
+		kfree_sensitive(ses->password);
+		ses->password = new_password;
 	}
+	if (new_password2) {
+		kfree_sensitive(ses->password2);
+		ses->password2 = new_password2;
+	}
+
+	mutex_unlock(&ses->session_mutex);
+
 	STEAL_STRING(cifs_sb, ctx, domainname);
 	STEAL_STRING(cifs_sb, ctx, nodename);
 	STEAL_STRING(cifs_sb, ctx, iocharset);
diff --git a/fs/smb/client/fs_context.h b/fs/smb/client/fs_context.h
index 890d6d9d4a59..503abb8f0b55 100644
--- a/fs/smb/client/fs_context.h
+++ b/fs/smb/client/fs_context.h
@@ -299,6 +299,7 @@ static inline struct smb3_fs_context *smb3_fc2context(const struct fs_context *f
 }
 
 extern int smb3_fs_context_dup(struct smb3_fs_context *new_ctx, struct smb3_fs_context *ctx);
+extern int smb_sync_session_ctx_passwords(struct cifs_sb_info *cifs_sb, struct cifs_ses *ses);
 extern void smb3_update_mnt_flags(struct cifs_sb_info *cifs_sb);
 
 /*
-- 
2.46.0.46.g406f326d27

Re: [PATCH 1/2] cifs: during remount, make sure passwords are in sync

[Meetakshi Setiya]

[-- Attachment #1: Type: text/plain, Size: 3909 bytes --]

The last patch needed some revision, please ignore it. Here is the
updated version:
Apologies for the confusion.

On Wed, Nov 13, 2024 at 5:18 PM Meetakshi Setiya
<meetakshisetiyaoss@gmail.com> wrote:
>
> Thanks for the review, Paulo. I have attached the updated patch.
> I will send the password rotation changes for DFS, SMB1 (and multiuser) later,
> separately.
>
> Best
> Meetakshi
>
> On Wed, Nov 13, 2024 at 3:30 PM Meetakshi Setiya
> <meetakshisetiyaoss@gmail.com> wrote:
> >
> > Typo: password rotation for SMB1.0 is NOT supported for reconnects. It works for fresh
> > mounts and remounts.
> > Should I add support for reconnect or remove it completely?
> >
> > On Wed, Nov 13, 2024 at 3:20 PM Meetakshi Setiya <meetakshisetiyaoss@gmail.com> wrote:
> >>
> >> Hi Paulo,
> >>
> >> Given your and Shyam's comments, I am thinking of making the
> >> following changes to the code to support password rotation for DFS:
> >>
> >> 1. For a fresh mount:
> >>     - In cifs_do_automount, bring the passwords in fs_context in sync with
> >>     the passwords in the session object before sending the context to the
> >>     child/submount.
> >>
> >> 2. For a remount (of the root only):
> >>     - In smb3_reconfigure, bring the passwords in the fs_context of the master
> >>     tcon in sync with its session object passwords. After this is done, a new
> >>     function will be called to iterate over the dfs_ses_list held in this tcon
> >>     and sync their session passwords with the updated root session password
> >>     and password2.
> >>
> >> Password rotation for multiuser mounts is out of scope for this patch and I will
> >> address it later.
> >>
> >> Please let me know if you have any comments or suggestions on this approach.
> >>
> >> Also, we share the mount code path (cifs_mount) for all SMB versions. So, password
> >> rotation for SMB1.0 is currently supported ONLY on mounts. Password rotation on
> >> remount, however, is not. Should I remove the support completely for SMB1.0
> >> (print a warning message), leave it be, or add remount support?
> >
> >
> > reconnect, not remount.
> >
> >>
> >> Thanks
> >> Meetakshi
> >>
> >> On Mon, Nov 11, 2024 at 5:34 PM Paulo Alcantara <pc@manguebit.com> wrote:
> >>>
> >>> Shyam Prasad N <nspmangalore@gmail.com> writes:
> >>>
> >>> > On Fri, Nov 8, 2024 at 5:47 PM Shyam Prasad N <nspmangalore@gmail.com> wrote:
> >>> >> > What about SMB sessions from cifs_tcon::dfs_ses_list?  I don't see their
> >>> >> > password getting updated over remount.
> >>> >>
> >>> >> This is in our to-do list as well.
> >>> >
> >>> > I did some code reading around how DFS automount works.
> >>> > @Paulo Alcantara Correct me if I'm wrong, but it sounds like we make
> >>> > an assumption that when a DFS namespace has a junction to another
> >>> > share, the same credentials are to be used to perform the mount of
> >>> > that share. Is that always the case?
> >>>
> >>> Yes, it inherits fs_context from the parent mount.  For multiuser
> >>> mounts, when uid/gid/cruid are unspecified, we need to update its values
> >>> to match real uid/gid from the calling process.
> >>>
> >>> > If we go by that assumption, for password2 to work with DFS mounts, we
> >>> > only need to make sure that in cifs_do_automount, cur_ctx passwords
> >>> > are synced up to the current ses passwords. That should be quite easy.
> >>>
> >>> Correct.  The fs_context for the automount is dup'ed from the parent
> >>> mount.  smb3_fs_context_dup() already dups password2, so it should work.
> >>>
> >>> The 'remount' case isn't still handled, that's why I mentioned it above.
> >>> You'd need to set password2 for all sessios in @tcon->dfs_ses_list.
> >>>
> >>> I think we need to update password2 for the multiuser sessions as well
> >>> and not only for session from master tcon.

[-- Attachment #2: 0001-cifs-during-remount-make-sure-passwords-are-in-sync.patch --]
[-- Type: application/octet-stream, Size: 5419 bytes --]

From e78cce9645721eb2b0616922c3353367334d3fb1 Mon Sep 17 00:00:00 2001
From: Shyam Prasad N <sprasad@microsoft.com>
Date: Wed, 30 Oct 2024 06:45:50 +0000
Subject: [PATCH 1/2] cifs: during remount, make sure passwords are in sync

We recently introduced a password2 field in both ses and ctx structs.
This was done so as to allow the client to rotate passwords for a mount
without any downtime. However, when the client transparently handles
password rotation, it can swap the values of the two password fields
in the ses struct, but not in smb3_fs_context struct that hangs off
cifs_sb. This can lead to a situation where a remount unintentionally
overwrites a working password in the ses struct.

In order to fix this, we first get the passwords in ctx struct
in-sync with ses struct, before replacing them with what the passwords
that could be passed as a part of remount.

Also, in order to avoid race condition between smb2_reconnect and
smb3_reconfigure, we make sure to lock session_mutex before changing
password and password2 fields of the ses structure.

Fixes: 35f834265e0d ("smb3: fix broken reconnect when password changing on the server by allowing password rotation")
Signed-off-by: Shyam Prasad N <sprasad@microsoft.com>
Signed-off-by: Meetakshi Setiya <msetiya@microsoft.com>
---
 fs/smb/client/fs_context.c | 83 +++++++++++++++++++++++++++++++++-----
 fs/smb/client/fs_context.h |  1 +
 2 files changed, 75 insertions(+), 9 deletions(-)

diff --git a/fs/smb/client/fs_context.c b/fs/smb/client/fs_context.c
index 5c5a52019efa..1a5842ced489 100644
--- a/fs/smb/client/fs_context.c
+++ b/fs/smb/client/fs_context.c
@@ -890,12 +890,37 @@ do {									\
 	cifs_sb->ctx->field = NULL;					\
 } while (0)
 
+int smb3_sync_session_ctx_passwords(struct cifs_sb_info *cifs_sb, struct cifs_ses *ses)
+{
+	if (ses->password &&
+	    cifs_sb->ctx->password &&
+	    strcmp(ses->password, cifs_sb->ctx->password)) {
+		kfree_sensitive(cifs_sb->ctx->password);
+		cifs_sb->ctx->password = kstrdup(ses->password, GFP_KERNEL);
+		if (!cifs_sb->ctx->password)
+			return -ENOMEM;
+	}
+	if (ses->password2 &&
+	    cifs_sb->ctx->password2 &&
+	    strcmp(ses->password2, cifs_sb->ctx->password2)) {
+		kfree_sensitive(cifs_sb->ctx->password2);
+		cifs_sb->ctx->password2 = kstrdup(ses->password2, GFP_KERNEL);
+		if (!cifs_sb->ctx->password2) {
+			kfree_sensitive(cifs_sb->ctx->password);
+			cifs_sb->ctx->password = NULL;
+			return -ENOMEM;
+		}
+	}
+	return 0;
+}
+
 static int smb3_reconfigure(struct fs_context *fc)
 {
 	struct smb3_fs_context *ctx = smb3_fc2context(fc);
 	struct dentry *root = fc->root;
 	struct cifs_sb_info *cifs_sb = CIFS_SB(root->d_sb);
 	struct cifs_ses *ses = cifs_sb_master_tcon(cifs_sb)->ses;
+	char *new_password = NULL, *new_password2 = NULL;
 	bool need_recon = false;
 	int rc;
 
@@ -915,21 +940,61 @@ static int smb3_reconfigure(struct fs_context *fc)
 	STEAL_STRING(cifs_sb, ctx, UNC);
 	STEAL_STRING(cifs_sb, ctx, source);
 	STEAL_STRING(cifs_sb, ctx, username);
+
 	if (need_recon == false)
 		STEAL_STRING_SENSITIVE(cifs_sb, ctx, password);
 	else  {
-		kfree_sensitive(ses->password);
-		ses->password = kstrdup(ctx->password, GFP_KERNEL);
-		if (!ses->password)
-			return -ENOMEM;
-		kfree_sensitive(ses->password2);
-		ses->password2 = kstrdup(ctx->password2, GFP_KERNEL);
-		if (!ses->password2) {
-			kfree_sensitive(ses->password);
-			ses->password = NULL;
+		if (ctx->password) {
+			new_password = kstrdup(ctx->password, GFP_KERNEL);
+			if (!new_password)
+				return -ENOMEM;
+		} else
+			STEAL_STRING_SENSITIVE(cifs_sb, ctx, password);
+	}
+
+	/*
+	 * if a new password2 has been specified, then reset it's value
+	 * inside the ses struct
+	 */
+	if (ctx->password2) {
+		new_password2 = kstrdup(ctx->password2, GFP_KERNEL);
+		if (!new_password2) {
+			kfree_sensitive(new_password);
 			return -ENOMEM;
 		}
+	} else
+		STEAL_STRING_SENSITIVE(cifs_sb, ctx, password2);
+
+	/*
+	 * we may update the passwords in the ses struct below. Make sure we do
+	 * not race with smb2_reconnect
+	 */
+	mutex_lock(&ses->session_mutex);
+
+	/*
+	 * smb2_reconnect may swap password and password2 in case session setup
+	 * failed. First get ctx passwords in sync with ses passwords. It should
+	 * be okay to do this even if this function were to return an error at a
+	 * later stage
+	 */
+	rc = smb3_sync_session_ctx_passwords(cifs_sb, ses);
+	if (rc)
+		return rc;
+
+	/*
+	 * now that allocations for passwords are done, commit them
+	 */
+	if (new_password) {
+		kfree_sensitive(ses->password);
+		ses->password = new_password;
 	}
+	if (new_password2) {
+		kfree_sensitive(ses->password2);
+		ses->password2 = new_password2;
+	}
+
+	mutex_unlock(&ses->session_mutex);
+
 	STEAL_STRING(cifs_sb, ctx, domainname);
 	STEAL_STRING(cifs_sb, ctx, nodename);
 	STEAL_STRING(cifs_sb, ctx, iocharset);
diff --git a/fs/smb/client/fs_context.h b/fs/smb/client/fs_context.h
index 890d6d9d4a59..c8c8b4451b3b 100644
--- a/fs/smb/client/fs_context.h
+++ b/fs/smb/client/fs_context.h
@@ -299,6 +299,7 @@ static inline struct smb3_fs_context *smb3_fc2context(const struct fs_context *f
 }
 
 extern int smb3_fs_context_dup(struct smb3_fs_context *new_ctx, struct smb3_fs_context *ctx);
+extern int smb3_sync_session_ctx_passwords(struct cifs_sb_info *cifs_sb, struct cifs_ses *ses);
 extern void smb3_update_mnt_flags(struct cifs_sb_info *cifs_sb);
 
 /*
-- 
2.46.0.46.g406f326d27

Re: [PATCH 1/2] cifs: during remount, make sure passwords are in sync

[Steve French]

My opinion is that since password rotation is important for various
security scenarios, there is value in improving it for all dialects,
especially as the missing piece in the SMB1 reconnect path shouldn't
be too hard to fix.

On Wed, Nov 13, 2024 at 4:00 AM Meetakshi Setiya
<meetakshisetiyaoss@gmail.com> wrote:
>
> Typo: password rotation for SMB1.0 is NOT supported for reconnects. It works for fresh
> mounts and remounts.
> Should I add support for reconnect or remove it completely?
>
> On Wed, Nov 13, 2024 at 3:20 PM Meetakshi Setiya <meetakshisetiyaoss@gmail.com> wrote:
>>
>> Hi Paulo,
>>
>> Given your and Shyam's comments, I am thinking of making the
>> following changes to the code to support password rotation for DFS:
>>
>> 1. For a fresh mount:
>>     - In cifs_do_automount, bring the passwords in fs_context in sync with
>>     the passwords in the session object before sending the context to the
>>     child/submount.
>>
>> 2. For a remount (of the root only):
>>     - In smb3_reconfigure, bring the passwords in the fs_context of the master
>>     tcon in sync with its session object passwords. After this is done, a new
>>     function will be called to iterate over the dfs_ses_list held in this tcon
>>     and sync their session passwords with the updated root session password
>>     and password2.
>>
>> Password rotation for multiuser mounts is out of scope for this patch and I will
>> address it later.
>>
>> Please let me know if you have any comments or suggestions on this approach.
>>
>> Also, we share the mount code path (cifs_mount) for all SMB versions. So, password
>> rotation for SMB1.0 is currently supported ONLY on mounts. Password rotation on
>> remount, however, is not. Should I remove the support completely for SMB1.0
>> (print a warning message), leave it be, or add remount support?
>
>
> reconnect, not remount.
>
>>
>> Thanks
>> Meetakshi
>>
>> On Mon, Nov 11, 2024 at 5:34 PM Paulo Alcantara <pc@manguebit.com> wrote:
>>>
>>> Shyam Prasad N <nspmangalore@gmail.com> writes:
>>>
>>> > On Fri, Nov 8, 2024 at 5:47 PM Shyam Prasad N <nspmangalore@gmail.com> wrote:
>>> >> > What about SMB sessions from cifs_tcon::dfs_ses_list?  I don't see their
>>> >> > password getting updated over remount.
>>> >>
>>> >> This is in our to-do list as well.
>>> >
>>> > I did some code reading around how DFS automount works.
>>> > @Paulo Alcantara Correct me if I'm wrong, but it sounds like we make
>>> > an assumption that when a DFS namespace has a junction to another
>>> > share, the same credentials are to be used to perform the mount of
>>> > that share. Is that always the case?
>>>
>>> Yes, it inherits fs_context from the parent mount.  For multiuser
>>> mounts, when uid/gid/cruid are unspecified, we need to update its values
>>> to match real uid/gid from the calling process.
>>>
>>> > If we go by that assumption, for password2 to work with DFS mounts, we
>>> > only need to make sure that in cifs_do_automount, cur_ctx passwords
>>> > are synced up to the current ses passwords. That should be quite easy.
>>>
>>> Correct.  The fs_context for the automount is dup'ed from the parent
>>> mount.  smb3_fs_context_dup() already dups password2, so it should work.
>>>
>>> The 'remount' case isn't still handled, that's why I mentioned it above.
>>> You'd need to set password2 for all sessios in @tcon->dfs_ses_list.
>>>
>>> I think we need to update password2 for the multiuser sessions as well
>>> and not only for session from master tcon.



-- 
Thanks,

Steve

Re: [PATCH 1/2] cifs: during remount, make sure passwords are in sync

[Jeremy Allison]

On Wed, Nov 13, 2024 at 10:51:26AM -0600, Steve French wrote:
>My opinion is that since password rotation is important for various
>security scenarios, there is value in improving it for all dialects,
>especially as the missing piece in the SMB1 reconnect path shouldn't
>be too hard to fix.

*ALL* effort in SMB1 is wasted effort. Please put it in
maintenence mode and just keep it compiling if needed,
but no new features.

Please.