On Fri, Nov 8, 2024 at 5:47 PM Shyam Prasad N <nspmangalore@xxxxxxxxx> wrote:
>
> Hi Paulo,
>
> Thanks for the review.
>
> On Fri, Nov 8, 2024 at 12:01 AM Paulo Alcantara <pc@xxxxxxxxxxxxx> wrote:
> >
> > meetakshisetiyaoss@xxxxxxxxx writes:
> >
> > > From: Shyam Prasad N <sprasad@xxxxxxxxxxxxx>
> > >
> > > 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@xxxxxxxxxxxxx>
> > > Signed-off-by: Meetakshi Setiya <msetiya@xxxxxxxxxxxxx>
> > > ---
> > > 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
