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.
>
> 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
