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

[Paulo Alcantara <pc@manguebit.com>]

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