| Message ID | 20241030142829.234828-1-meetakshisetiyaoss@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/2] cifs: during remount, make sure passwords are in sync | expand |
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
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
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
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 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.
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.
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.
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.
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);