diff mbox series

[14/21] cifs: we do not allow changing username/password/unc/... during remount

Message ID 20201207233646.29823-14-lsahlber@redhat.com (mailing list archive)
State New, archived
Headers show
Series None | expand

Commit Message

Ronnie Sahlberg Dec. 7, 2020, 11:36 p.m. UTC 
Signed-off-by: Ronnie Sahlberg <lsahlber@redhat.com>
---
 fs/cifs/cifsfs.c     |  2 +-
 fs/cifs/fs_context.c | 55 +++++++++++++++++++++++++++++++++++++++++++++++++---
 fs/cifs/fs_context.h |  2 +-
 3 files changed, 54 insertions(+), 5 deletions(-)

Comments

Steve French Dec. 8, 2020, 5:06 a.m. UTC | #1 
Minor nits pointed out by checkpatch:

0015-cifs-we-do-not-allow-changing-username-password-unc-.patch
---------------------------------------------------------------
WARNING: Missing commit description - Add an appropriate one

WARNING: kfree(NULL) is safe and this check is probably not required
#76: FILE: fs/cifs/fs_context.c:673:
+ if (ctx->field) { \
+ kfree(ctx->field);

On Mon, Dec 7, 2020 at 5:37 PM Ronnie Sahlberg <lsahlber@redhat.com> wrote:
>
> Signed-off-by: Ronnie Sahlberg <lsahlber@redhat.com>
> ---
>  fs/cifs/cifsfs.c     |  2 +-
>  fs/cifs/fs_context.c | 55 +++++++++++++++++++++++++++++++++++++++++++++++++---
>  fs/cifs/fs_context.h |  2 +-
>  3 files changed, 54 insertions(+), 5 deletions(-)
>
> diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c
> index 80117e9d35f9..13d7f4a3c836 100644
> --- a/fs/cifs/cifsfs.c
> +++ b/fs/cifs/cifsfs.c
> @@ -490,7 +490,7 @@ cifs_show_options(struct seq_file *s, struct dentry *root)
>
>         if (tcon->no_lease)
>                 seq_puts(s, ",nolease");
> -       if (cifs_sb->mnt_cifs_flags & CIFS_MOUNT_MULTIUSER)
> +       if (cifs_sb->ctx->multiuser)
>                 seq_puts(s, ",multiuser");
>         else if (tcon->ses->user_name)
>                 seq_show_option(s, "username", tcon->ses->user_name);
> diff --git a/fs/cifs/fs_context.c b/fs/cifs/fs_context.c
> index edfdea129fcc..542fa75b74aa 100644
> --- a/fs/cifs/fs_context.c
> +++ b/fs/cifs/fs_context.c
> @@ -629,10 +629,53 @@ static int smb3_verify_reconfigure_ctx(struct smb3_fs_context *new_ctx,
>                 cifs_dbg(VFS, "can not change sec during remount\n");
>                 return -EINVAL;
>         }
> +       if (new_ctx->multiuser != old_ctx->multiuser) {
> +               cifs_dbg(VFS, "can not change multiuser during remount\n");
> +               return -EINVAL;
> +       }
> +       if (new_ctx->UNC &&
> +           (!old_ctx->UNC || strcmp(new_ctx->UNC, old_ctx->UNC))) {
> +               cifs_dbg(VFS, "can not change UNC during remount\n");
> +               return -EINVAL;
> +       }
> +       if (new_ctx->username &&
> +           (!old_ctx->username || strcmp(new_ctx->username, old_ctx->username))) {
> +               cifs_dbg(VFS, "can not change username during remount\n");
> +               return -EINVAL;
> +       }
> +       if (new_ctx->password &&
> +           (!old_ctx->password || strcmp(new_ctx->password, old_ctx->password))) {
> +               cifs_dbg(VFS, "can not change password during remount\n");
> +               return -EINVAL;
> +       }
> +       if (new_ctx->domainname &&
> +           (!old_ctx->domainname || strcmp(new_ctx->domainname, old_ctx->domainname))) {
> +               cifs_dbg(VFS, "can not change domainname during remount\n");
> +               return -EINVAL;
> +       }
> +       if (new_ctx->nodename &&
> +           (!old_ctx->nodename || strcmp(new_ctx->nodename, old_ctx->nodename))) {
> +               cifs_dbg(VFS, "can not change nodename during remount\n");
> +               return -EINVAL;
> +       }
> +       if (new_ctx->iocharset &&
> +           (!old_ctx->iocharset || strcmp(new_ctx->iocharset, old_ctx->iocharset))) {
> +               cifs_dbg(VFS, "can not change iocharset during remount\n");
> +               return -EINVAL;
> +       }
>
>         return 0;
>  }
>
> +#define STEAL_STRING(cifs_sb, ctx, field)                              \
> +do {                                                                   \
> +       if (ctx->field) {                                               \
> +               kfree(ctx->field);                                      \
> +               ctx->field = cifs_sb->ctx->field;                       \
> +               cifs_sb->ctx->field = NULL;                             \
> +       }                                                               \
> +} while (0)
> +
>  static int smb3_reconfigure(struct fs_context *fc)
>  {
>         struct smb3_fs_context *ctx = smb3_fc2context(fc);
> @@ -645,10 +688,16 @@ static int smb3_reconfigure(struct fs_context *fc)
>                 return rc;
>
>         /*
> -        * Steal the UNC from the old and to be destroyed context.
> +        * We can not change UNC/username/password/domainname/nodename/iocharset
> +        * during reconnect so ignore what we have in the new context and
> +        * just use what we already have in cifs_sb->ctx.
>          */
> -       ctx->UNC = cifs_sb->ctx->UNC;
> -       cifs_sb->ctx->UNC = NULL;
> +       STEAL_STRING(cifs_sb, ctx, UNC);
> +       STEAL_STRING(cifs_sb, ctx, username);
> +       STEAL_STRING(cifs_sb, ctx, password);
> +       STEAL_STRING(cifs_sb, ctx, domainname);
> +       STEAL_STRING(cifs_sb, ctx, nodename);
> +       STEAL_STRING(cifs_sb, ctx, iocharset);
>
>         smb3_cleanup_fs_context_contents(cifs_sb->ctx);
>         rc = smb3_fs_context_dup(cifs_sb->ctx, ctx);
> diff --git a/fs/cifs/fs_context.h b/fs/cifs/fs_context.h
> index aa1d952fd5ce..62f5a8d98df6 100644
> --- a/fs/cifs/fs_context.h
> +++ b/fs/cifs/fs_context.h
> @@ -148,7 +148,6 @@ struct smb3_fs_context {
>         bool uid_specified;
>         bool gid_specified;
>         bool sloppy;
> -       char *nodename;
>         bool got_ip;
>         bool got_version;
>         bool got_rsize;
> @@ -160,6 +159,7 @@ struct smb3_fs_context {
>         char *password;
>         char *domainname;
>         char *UNC;
> +       char *nodename;
>         char *iocharset;  /* local code page for mapping to and from Unicode */
>         char source_rfc1001_name[RFC1001_NAME_LEN_WITH_NULL]; /* clnt nb name */
>         char target_rfc1001_name[RFC1001_NAME_LEN_WITH_NULL]; /* srvr nb name */
> --
> 2.13.6
>
Pavel Shilovsky Dec. 8, 2020, 6:42 p.m. UTC | #2 
пн, 7 дек. 2020 г. в 21:07, Steve French <smfrench@gmail.com>:
>
> Minor nits pointed out by checkpatch:
>
> 0015-cifs-we-do-not-allow-changing-username-password-unc-.patch
> ---------------------------------------------------------------
> WARNING: Missing commit description - Add an appropriate one
>
> WARNING: kfree(NULL) is safe and this check is probably not required
> #76: FILE: fs/cifs/fs_context.c:673:
> + if (ctx->field) { \
> + kfree(ctx->field);
>
> On Mon, Dec 7, 2020 at 5:37 PM Ronnie Sahlberg <lsahlber@redhat.com> wrote:
> >
> > Signed-off-by: Ronnie Sahlberg <lsahlber@redhat.com>
> > ---
> >  fs/cifs/cifsfs.c     |  2 +-
> >  fs/cifs/fs_context.c | 55 +++++++++++++++++++++++++++++++++++++++++++++++++---
> >  fs/cifs/fs_context.h |  2 +-
> >  3 files changed, 54 insertions(+), 5 deletions(-)
> >
> > diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c
> > index 80117e9d35f9..13d7f4a3c836 100644
> > --- a/fs/cifs/cifsfs.c
> > +++ b/fs/cifs/cifsfs.c
> > @@ -490,7 +490,7 @@ cifs_show_options(struct seq_file *s, struct dentry *root)
> >
> >         if (tcon->no_lease)
> >                 seq_puts(s, ",nolease");
> > -       if (cifs_sb->mnt_cifs_flags & CIFS_MOUNT_MULTIUSER)
> > +       if (cifs_sb->ctx->multiuser)
> >                 seq_puts(s, ",multiuser");
> >         else if (tcon->ses->user_name)
> >                 seq_show_option(s, "username", tcon->ses->user_name);
> > diff --git a/fs/cifs/fs_context.c b/fs/cifs/fs_context.c
> > index edfdea129fcc..542fa75b74aa 100644
> > --- a/fs/cifs/fs_context.c
> > +++ b/fs/cifs/fs_context.c
> > @@ -629,10 +629,53 @@ static int smb3_verify_reconfigure_ctx(struct smb3_fs_context *new_ctx,
> >                 cifs_dbg(VFS, "can not change sec during remount\n");
> >                 return -EINVAL;
> >         }
> > +       if (new_ctx->multiuser != old_ctx->multiuser) {
> > +               cifs_dbg(VFS, "can not change multiuser during remount\n");
> > +               return -EINVAL;
> > +       }
> > +       if (new_ctx->UNC &&
> > +           (!old_ctx->UNC || strcmp(new_ctx->UNC, old_ctx->UNC))) {
> > +               cifs_dbg(VFS, "can not change UNC during remount\n");
> > +               return -EINVAL;
> > +       }
> > +       if (new_ctx->username &&
> > +           (!old_ctx->username || strcmp(new_ctx->username, old_ctx->username))) {
> > +               cifs_dbg(VFS, "can not change username during remount\n");
> > +               return -EINVAL;
> > +       }
> > +       if (new_ctx->password &&
> > +           (!old_ctx->password || strcmp(new_ctx->password, old_ctx->password))) {
> > +               cifs_dbg(VFS, "can not change password during remount\n");
> > +               return -EINVAL;
> > +       }
> > +       if (new_ctx->domainname &&
> > +           (!old_ctx->domainname || strcmp(new_ctx->domainname, old_ctx->domainname))) {
> > +               cifs_dbg(VFS, "can not change domainname during remount\n");
> > +               return -EINVAL;
> > +       }
> > +       if (new_ctx->nodename &&
> > +           (!old_ctx->nodename || strcmp(new_ctx->nodename, old_ctx->nodename))) {
> > +               cifs_dbg(VFS, "can not change nodename during remount\n");
> > +               return -EINVAL;
> > +       }
> > +       if (new_ctx->iocharset &&
> > +           (!old_ctx->iocharset || strcmp(new_ctx->iocharset, old_ctx->iocharset))) {
> > +               cifs_dbg(VFS, "can not change iocharset during remount\n");
> > +               return -EINVAL;
> > +       }
> >
> >         return 0;
> >  }
> >
> > +#define STEAL_STRING(cifs_sb, ctx, field)                              \
> > +do {                                                                   \
> > +       if (ctx->field) {                                               \
> > +               kfree(ctx->field);                                      \
> > +               ctx->field = cifs_sb->ctx->field;                       \
> > +               cifs_sb->ctx->field = NULL;                             \
> > +       }                                                               \
> > +} while (0)

If ctx->field is NULL we won't assign new value from
cifs_sb->ctx->field and the procedure will become no-op. Is this an
intent?

--
Best regards,
Pavel Shilovsky
ronnie sahlberg Dec. 8, 2020, 9:38 p.m. UTC | #3 
On Wed, Dec 9, 2020 at 7:08 AM Pavel Shilovsky <piastryyy@gmail.com> wrote:
>
> пн, 7 дек. 2020 г. в 21:07, Steve French <smfrench@gmail.com>:
> >
> > Minor nits pointed out by checkpatch:
> >
> > 0015-cifs-we-do-not-allow-changing-username-password-unc-.patch
> > ---------------------------------------------------------------
> > WARNING: Missing commit description - Add an appropriate one
> >
> > WARNING: kfree(NULL) is safe and this check is probably not required
> > #76: FILE: fs/cifs/fs_context.c:673:
> > + if (ctx->field) { \
> > + kfree(ctx->field);
> >
> > On Mon, Dec 7, 2020 at 5:37 PM Ronnie Sahlberg <lsahlber@redhat.com> wrote:
> > >
> > > Signed-off-by: Ronnie Sahlberg <lsahlber@redhat.com>
> > > ---
> > >  fs/cifs/cifsfs.c     |  2 +-
> > >  fs/cifs/fs_context.c | 55 +++++++++++++++++++++++++++++++++++++++++++++++++---
> > >  fs/cifs/fs_context.h |  2 +-
> > >  3 files changed, 54 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c
> > > index 80117e9d35f9..13d7f4a3c836 100644
> > > --- a/fs/cifs/cifsfs.c
> > > +++ b/fs/cifs/cifsfs.c
> > > @@ -490,7 +490,7 @@ cifs_show_options(struct seq_file *s, struct dentry *root)
> > >
> > >         if (tcon->no_lease)
> > >                 seq_puts(s, ",nolease");
> > > -       if (cifs_sb->mnt_cifs_flags & CIFS_MOUNT_MULTIUSER)
> > > +       if (cifs_sb->ctx->multiuser)
> > >                 seq_puts(s, ",multiuser");
> > >         else if (tcon->ses->user_name)
> > >                 seq_show_option(s, "username", tcon->ses->user_name);
> > > diff --git a/fs/cifs/fs_context.c b/fs/cifs/fs_context.c
> > > index edfdea129fcc..542fa75b74aa 100644
> > > --- a/fs/cifs/fs_context.c
> > > +++ b/fs/cifs/fs_context.c
> > > @@ -629,10 +629,53 @@ static int smb3_verify_reconfigure_ctx(struct smb3_fs_context *new_ctx,
> > >                 cifs_dbg(VFS, "can not change sec during remount\n");
> > >                 return -EINVAL;
> > >         }
> > > +       if (new_ctx->multiuser != old_ctx->multiuser) {
> > > +               cifs_dbg(VFS, "can not change multiuser during remount\n");
> > > +               return -EINVAL;
> > > +       }
> > > +       if (new_ctx->UNC &&
> > > +           (!old_ctx->UNC || strcmp(new_ctx->UNC, old_ctx->UNC))) {
> > > +               cifs_dbg(VFS, "can not change UNC during remount\n");
> > > +               return -EINVAL;
> > > +       }
> > > +       if (new_ctx->username &&
> > > +           (!old_ctx->username || strcmp(new_ctx->username, old_ctx->username))) {
> > > +               cifs_dbg(VFS, "can not change username during remount\n");
> > > +               return -EINVAL;
> > > +       }
> > > +       if (new_ctx->password &&
> > > +           (!old_ctx->password || strcmp(new_ctx->password, old_ctx->password))) {
> > > +               cifs_dbg(VFS, "can not change password during remount\n");
> > > +               return -EINVAL;
> > > +       }
> > > +       if (new_ctx->domainname &&
> > > +           (!old_ctx->domainname || strcmp(new_ctx->domainname, old_ctx->domainname))) {
> > > +               cifs_dbg(VFS, "can not change domainname during remount\n");
> > > +               return -EINVAL;
> > > +       }
> > > +       if (new_ctx->nodename &&
> > > +           (!old_ctx->nodename || strcmp(new_ctx->nodename, old_ctx->nodename))) {
> > > +               cifs_dbg(VFS, "can not change nodename during remount\n");
> > > +               return -EINVAL;
> > > +       }
> > > +       if (new_ctx->iocharset &&
> > > +           (!old_ctx->iocharset || strcmp(new_ctx->iocharset, old_ctx->iocharset))) {
> > > +               cifs_dbg(VFS, "can not change iocharset during remount\n");
> > > +               return -EINVAL;
> > > +       }
> > >
> > >         return 0;
> > >  }
> > >
> > > +#define STEAL_STRING(cifs_sb, ctx, field)                              \
> > > +do {                                                                   \
> > > +       if (ctx->field) {                                               \
> > > +               kfree(ctx->field);                                      \
> > > +               ctx->field = cifs_sb->ctx->field;                       \
> > > +               cifs_sb->ctx->field = NULL;                             \
> > > +       }                                                               \
> > > +} while (0)
>
> If ctx->field is NULL we won't assign new value from
> cifs_sb->ctx->field and the procedure will become no-op. Is this an
> intent?

No, that is a bug. I will fix that.

Thanks.

>
> --
> Best regards,
> Pavel Shilovsky
diff mbox series

Patch

diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c
index 80117e9d35f9..13d7f4a3c836 100644
--- a/fs/cifs/cifsfs.c
+++ b/fs/cifs/cifsfs.c
@@ -490,7 +490,7 @@  cifs_show_options(struct seq_file *s, struct dentry *root)
 
 	if (tcon->no_lease)
 		seq_puts(s, ",nolease");
-	if (cifs_sb->mnt_cifs_flags & CIFS_MOUNT_MULTIUSER)
+	if (cifs_sb->ctx->multiuser)
 		seq_puts(s, ",multiuser");
 	else if (tcon->ses->user_name)
 		seq_show_option(s, "username", tcon->ses->user_name);
diff --git a/fs/cifs/fs_context.c b/fs/cifs/fs_context.c
index edfdea129fcc..542fa75b74aa 100644
--- a/fs/cifs/fs_context.c
+++ b/fs/cifs/fs_context.c
@@ -629,10 +629,53 @@  static int smb3_verify_reconfigure_ctx(struct smb3_fs_context *new_ctx,
 		cifs_dbg(VFS, "can not change sec during remount\n");
 		return -EINVAL;
 	}
+	if (new_ctx->multiuser != old_ctx->multiuser) {
+		cifs_dbg(VFS, "can not change multiuser during remount\n");
+		return -EINVAL;
+	}
+	if (new_ctx->UNC &&
+	    (!old_ctx->UNC || strcmp(new_ctx->UNC, old_ctx->UNC))) {
+		cifs_dbg(VFS, "can not change UNC during remount\n");
+		return -EINVAL;
+	}
+	if (new_ctx->username &&
+	    (!old_ctx->username || strcmp(new_ctx->username, old_ctx->username))) {
+		cifs_dbg(VFS, "can not change username during remount\n");
+		return -EINVAL;
+	}
+	if (new_ctx->password &&
+	    (!old_ctx->password || strcmp(new_ctx->password, old_ctx->password))) {
+		cifs_dbg(VFS, "can not change password during remount\n");
+		return -EINVAL;
+	}
+	if (new_ctx->domainname &&
+	    (!old_ctx->domainname || strcmp(new_ctx->domainname, old_ctx->domainname))) {
+		cifs_dbg(VFS, "can not change domainname during remount\n");
+		return -EINVAL;
+	}
+	if (new_ctx->nodename &&
+	    (!old_ctx->nodename || strcmp(new_ctx->nodename, old_ctx->nodename))) {
+		cifs_dbg(VFS, "can not change nodename during remount\n");
+		return -EINVAL;
+	}
+	if (new_ctx->iocharset &&
+	    (!old_ctx->iocharset || strcmp(new_ctx->iocharset, old_ctx->iocharset))) {
+		cifs_dbg(VFS, "can not change iocharset during remount\n");
+		return -EINVAL;
+	}
 
 	return 0;
 }
 
+#define STEAL_STRING(cifs_sb, ctx, field)				\
+do {									\
+	if (ctx->field) {						\
+		kfree(ctx->field);					\
+		ctx->field = cifs_sb->ctx->field;			\
+		cifs_sb->ctx->field = NULL;				\
+	}								\
+} while (0)
+
 static int smb3_reconfigure(struct fs_context *fc)
 {
 	struct smb3_fs_context *ctx = smb3_fc2context(fc);
@@ -645,10 +688,16 @@  static int smb3_reconfigure(struct fs_context *fc)
 		return rc;
 
 	/*
-	 * Steal the UNC from the old and to be destroyed context.
+	 * We can not change UNC/username/password/domainname/nodename/iocharset
+	 * during reconnect so ignore what we have in the new context and
+	 * just use what we already have in cifs_sb->ctx.
 	 */
-	ctx->UNC = cifs_sb->ctx->UNC;
-	cifs_sb->ctx->UNC = NULL;
+	STEAL_STRING(cifs_sb, ctx, UNC);
+	STEAL_STRING(cifs_sb, ctx, username);
+	STEAL_STRING(cifs_sb, ctx, password);
+	STEAL_STRING(cifs_sb, ctx, domainname);
+	STEAL_STRING(cifs_sb, ctx, nodename);
+	STEAL_STRING(cifs_sb, ctx, iocharset);
 
 	smb3_cleanup_fs_context_contents(cifs_sb->ctx);
 	rc = smb3_fs_context_dup(cifs_sb->ctx, ctx);
diff --git a/fs/cifs/fs_context.h b/fs/cifs/fs_context.h
index aa1d952fd5ce..62f5a8d98df6 100644
--- a/fs/cifs/fs_context.h
+++ b/fs/cifs/fs_context.h
@@ -148,7 +148,6 @@  struct smb3_fs_context {
 	bool uid_specified;
 	bool gid_specified;
 	bool sloppy;
-	char *nodename;
 	bool got_ip;
 	bool got_version;
 	bool got_rsize;
@@ -160,6 +159,7 @@  struct smb3_fs_context {
 	char *password;
 	char *domainname;
 	char *UNC;
+	char *nodename;
 	char *iocharset;  /* local code page for mapping to and from Unicode */
 	char source_rfc1001_name[RFC1001_NAME_LEN_WITH_NULL]; /* clnt nb name */
 	char target_rfc1001_name[RFC1001_NAME_LEN_WITH_NULL]; /* srvr nb name */