[PATCH] smb: client: fix regression with guest option

[PATCH] smb: client: fix regression with guest option

[Paulo Alcantara]

When mounting a CIFS share with 'guest' mount option, mount.cifs(8)
will set empty password= and password2= options.  Currently we only
handle empty strings from user= and password= options, so the mount
will fail with

	cifs: Bad value for 'password2'

Fix this by handling empty string from password2= option as well.

Link: https://bbs.archlinux.org/viewtopic.php?id=303927
Reported-by: Adam Williamson <awilliam@redhat.com>
Closes: https://lore.kernel.org/r/83c00b5fea81c07f6897a5dd3ef50fd3b290f56c.camel@redhat.com
Fixes: 35f834265e0d ("smb3: fix broken reconnect when password changing on the server by allowing password rotation")
Signed-off-by: Paulo Alcantara (Red Hat) <pc@manguebit.com>
---
 fs/smb/client/fs_context.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/fs/smb/client/fs_context.c b/fs/smb/client/fs_context.c
index da5973c228ed..8c73d4d60d1a 100644
--- a/fs/smb/client/fs_context.c
+++ b/fs/smb/client/fs_context.c
@@ -171,6 +171,7 @@ const struct fs_parameter_spec smb3_fs_parameters[] = {
 	fsparam_string("username", Opt_user),
 	fsparam_string("pass", Opt_pass),
 	fsparam_string("password", Opt_pass),
+	fsparam_string("pass2", Opt_pass2),
 	fsparam_string("password2", Opt_pass2),
 	fsparam_string("ip", Opt_ip),
 	fsparam_string("addr", Opt_ip),
@@ -1131,6 +1132,9 @@ static int smb3_fs_context_parse_param(struct fs_context *fc,
 		} else if (!strcmp("user", param->key) || !strcmp("username", param->key)) {
 			skip_parsing = true;
 			opt = Opt_user;
+		} else if (!strcmp("pass2", param->key) || !strcmp("password2", param->key)) {
+			skip_parsing = true;
+			opt = Opt_pass2;
 		}
 	}
 
-- 
2.48.1

Re: [PATCH] smb: client: fix regression with guest option

[Steve French]

merged into cifs-2.6.git for-next pending additional testing/review

Presumably we could also update cifs-utils (mount.cifs.c) to
workaround this as well.  Thoughts?

On Wed, Mar 12, 2025 at 8:51 AM Paulo Alcantara <pc@manguebit.com> wrote:
>
> When mounting a CIFS share with 'guest' mount option, mount.cifs(8)
> will set empty password= and password2= options.  Currently we only
> handle empty strings from user= and password= options, so the mount
> will fail with
>
>         cifs: Bad value for 'password2'
>
> Fix this by handling empty string from password2= option as well.
>
> Link: https://bbs.archlinux.org/viewtopic.php?id=303927
> Reported-by: Adam Williamson <awilliam@redhat.com>
> Closes: https://lore.kernel.org/r/83c00b5fea81c07f6897a5dd3ef50fd3b290f56c.camel@redhat.com
> Fixes: 35f834265e0d ("smb3: fix broken reconnect when password changing on the server by allowing password rotation")
> Signed-off-by: Paulo Alcantara (Red Hat) <pc@manguebit.com>
> ---
>  fs/smb/client/fs_context.c | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/fs/smb/client/fs_context.c b/fs/smb/client/fs_context.c
> index da5973c228ed..8c73d4d60d1a 100644
> --- a/fs/smb/client/fs_context.c
> +++ b/fs/smb/client/fs_context.c
> @@ -171,6 +171,7 @@ const struct fs_parameter_spec smb3_fs_parameters[] = {
>         fsparam_string("username", Opt_user),
>         fsparam_string("pass", Opt_pass),
>         fsparam_string("password", Opt_pass),
> +       fsparam_string("pass2", Opt_pass2),
>         fsparam_string("password2", Opt_pass2),
>         fsparam_string("ip", Opt_ip),
>         fsparam_string("addr", Opt_ip),
> @@ -1131,6 +1132,9 @@ static int smb3_fs_context_parse_param(struct fs_context *fc,
>                 } else if (!strcmp("user", param->key) || !strcmp("username", param->key)) {
>                         skip_parsing = true;
>                         opt = Opt_user;
> +               } else if (!strcmp("pass2", param->key) || !strcmp("password2", param->key)) {
> +                       skip_parsing = true;
> +                       opt = Opt_pass2;
>                 }
>         }
>
> --
> 2.48.1
>


-- 
Thanks,

Steve

Re: [PATCH] smb: client: fix regression with guest option

[Adam Williamson]

On Wed, 2025-03-12 at 10:32 -0500, Steve French wrote:
> merged into cifs-2.6.git for-next pending additional testing/review
> 
> Presumably we could also update cifs-utils (mount.cifs.c) to
> workaround this as well.  Thoughts?

Yeah, I was going to ask whether just not passing password2 when doing
anonymous mount might be an option? That way the new cifs-utils will
work with older kernels, rather than being a sudden surprise for anyone
who happens to get a new cifs-utils but not a new kernel, for whatever
reason...
-- 
Adam Williamson (he/him/his)
Fedora QA
Fedora Chat: @adamwill:fedora.im | Mastodon: @adamw@fosstodon.org
https://www.happyassassin.net

Re: [PATCH] smb: client: fix regression with guest option

[Paulo Alcantara]

Adam Williamson <awilliam@redhat.com> writes:

> On Wed, 2025-03-12 at 10:32 -0500, Steve French wrote:
>> merged into cifs-2.6.git for-next pending additional testing/review
>> 
>> Presumably we could also update cifs-utils (mount.cifs.c) to
>> workaround this as well.  Thoughts?
>
> Yeah, I was going to ask whether just not passing password2 when doing
> anonymous mount might be an option? That way the new cifs-utils will
> work with older kernels, rather than being a sudden surprise for anyone
> who happens to get a new cifs-utils but not a new kernel, for whatever

Yes, we could also change cifs-utils to not send both password= and
password2= when using guest mount option.

The kernel change is still required in case someone is using mount(2)
directly and end up passing an empty password2= option.

Re: [PATCH] smb: client: fix regression with guest option

[Steve French]

Meetakshi sent a patch idea to try (to also fix this in cifs-utils) -
will take a look

On Wed, Mar 12, 2025 at 11:02 AM Paulo Alcantara <pc@manguebit.com> wrote:
>
> Adam Williamson <awilliam@redhat.com> writes:
>
> > On Wed, 2025-03-12 at 10:32 -0500, Steve French wrote:
> >> merged into cifs-2.6.git for-next pending additional testing/review
> >>
> >> Presumably we could also update cifs-utils (mount.cifs.c) to
> >> workaround this as well.  Thoughts?
> >
> > Yeah, I was going to ask whether just not passing password2 when doing
> > anonymous mount might be an option? That way the new cifs-utils will
> > work with older kernels, rather than being a sudden surprise for anyone
> > who happens to get a new cifs-utils but not a new kernel, for whatever
>
> Yes, we could also change cifs-utils to not send both password= and
> password2= when using guest mount option.
>
> The kernel change is still required in case someone is using mount(2)
> directly and end up passing an empty password2= option.



-- 
Thanks,

Steve

Re: [PATCH] smb: client: fix regression with guest option

[Paulo Alcantara]

Steve French <smfrench@gmail.com> writes:

> Meetakshi sent a patch idea to try (to also fix this in cifs-utils) -
> will take a look

Where is the patch?

Something like below would work

diff --git a/mount.cifs.c b/mount.cifs.c
index 7605130..16730c6 100644
--- a/mount.cifs.c
+++ b/mount.cifs.c
@@ -200,6 +200,7 @@ struct parsed_mount_info {
        unsigned int got_domain:1;
        unsigned int is_krb5:1;
        unsigned int is_noauth:1;
+       unsigned int is_guest:1;
        uid_t sudo_uid;
 };
 
@@ -1161,6 +1162,7 @@ parse_options(const char *data, struct parsed_mount_info *parsed_info)
                        parsed_info->got_user = 1;
                        parsed_info->got_password = 1;
                        parsed_info->got_password2 = 1;
+                       parsed_info->is_guest = 1;
                        goto nocopy;
                case OPT_RO:
                        *filesys_flags |= MS_RDONLY;
@@ -2334,7 +2336,9 @@ mount_retry:
                fprintf(stderr, "%s kernel mount options: %s",
                        thisprogram, options);
 
-       if (parsed_info->got_password && !(parsed_info->is_krb5 || parsed_info->is_noauth)) {
+       if (parsed_info->got_password &&
+           !(parsed_info->is_krb5 || parsed_info->is_noauth ||
+             parsed_info->is_guest)) {
                /*
                 * Commas have to be doubled, or else they will
                 * look like the parameter separator
@@ -2345,7 +2349,9 @@ mount_retry:
                        fprintf(stderr, ",pass=********");
        }
 
-       if (parsed_info->got_password2 && !(parsed_info->is_krb5 || parsed_info->is_noauth)) {
+       if (parsed_info->got_password2 &&
+           !(parsed_info->is_krb5 || parsed_info->is_noauth ||
+             parsed_info->is_guest)) {
                strlcat(options, ",password2=", options_size);
                strlcat(options, parsed_info->password2, options_size);
                if (parsed_info->verboseflag)

Re: [PATCH] smb: client: fix regression with guest option

[Meetakshi Setiya]

Thanks Paulo, created a PR for cifs-utils based on your suggestion
https://github.com/smfrench/smb3-utils/pull/14

Thanks
Meetakshi

On Wed, Mar 12, 2025 at 9:53 PM Paulo Alcantara <pc@manguebit.com> wrote:
>
> Steve French <smfrench@gmail.com> writes:
>
> > Meetakshi sent a patch idea to try (to also fix this in cifs-utils) -
> > will take a look
>
> Where is the patch?
>
> Something like below would work
>
> diff --git a/mount.cifs.c b/mount.cifs.c
> index 7605130..16730c6 100644
> --- a/mount.cifs.c
> +++ b/mount.cifs.c
> @@ -200,6 +200,7 @@ struct parsed_mount_info {
>         unsigned int got_domain:1;
>         unsigned int is_krb5:1;
>         unsigned int is_noauth:1;
> +       unsigned int is_guest:1;
>         uid_t sudo_uid;
>  };
>
> @@ -1161,6 +1162,7 @@ parse_options(const char *data, struct parsed_mount_info *parsed_info)
>                         parsed_info->got_user = 1;
>                         parsed_info->got_password = 1;
>                         parsed_info->got_password2 = 1;
> +                       parsed_info->is_guest = 1;
>                         goto nocopy;
>                 case OPT_RO:
>                         *filesys_flags |= MS_RDONLY;
> @@ -2334,7 +2336,9 @@ mount_retry:
>                 fprintf(stderr, "%s kernel mount options: %s",
>                         thisprogram, options);
>
> -       if (parsed_info->got_password && !(parsed_info->is_krb5 || parsed_info->is_noauth)) {
> +       if (parsed_info->got_password &&
> +           !(parsed_info->is_krb5 || parsed_info->is_noauth ||
> +             parsed_info->is_guest)) {
>                 /*
>                  * Commas have to be doubled, or else they will
>                  * look like the parameter separator
> @@ -2345,7 +2349,9 @@ mount_retry:
>                         fprintf(stderr, ",pass=********");
>         }
>
> -       if (parsed_info->got_password2 && !(parsed_info->is_krb5 || parsed_info->is_noauth)) {
> +       if (parsed_info->got_password2 &&
> +           !(parsed_info->is_krb5 || parsed_info->is_noauth ||
> +             parsed_info->is_guest)) {
>                 strlcat(options, ",password2=", options_size);
>                 strlcat(options, parsed_info->password2, options_size);
>                 if (parsed_info->verboseflag)

Re: [PATCH] smb: client: fix regression with guest option

[Paulo Alcantara]

Meetakshi Setiya <meetakshisetiyaoss@gmail.com> writes:

> Thanks Paulo, created a PR for cifs-utils based on your suggestion
> https://github.com/smfrench/smb3-utils/pull/14

LGTM.  Thanks!

Re: [PATCH] smb: client: fix regression with guest option

[Steve French]

[-- Attachment #1: Type: text/plain, Size: 2865 bytes --]

Made minor updates to the patch (white space cleanup that checkpatch
spotted) and added the RB (see attached)

Merged into github smb3-utils for-next branch and also to cifs-utils
samba.org for-next
https://git.samba.org/?p=cifs-utils.git;a=shortlog;h=refs/heads/for-next

On Sat, Mar 15, 2025 at 2:09 AM Meetakshi Setiya
<meetakshisetiyaoss@gmail.com> wrote:
>
> Thanks Paulo, created a PR for cifs-utils based on your suggestion
> https://github.com/smfrench/smb3-utils/pull/14
>
> Thanks
> Meetakshi
>
> On Wed, Mar 12, 2025 at 9:53 PM Paulo Alcantara <pc@manguebit.com> wrote:
> >
> > Steve French <smfrench@gmail.com> writes:
> >
> > > Meetakshi sent a patch idea to try (to also fix this in cifs-utils) -
> > > will take a look
> >
> > Where is the patch?
> >
> > Something like below would work
> >
> > diff --git a/mount.cifs.c b/mount.cifs.c
> > index 7605130..16730c6 100644
> > --- a/mount.cifs.c
> > +++ b/mount.cifs.c
> > @@ -200,6 +200,7 @@ struct parsed_mount_info {
> >         unsigned int got_domain:1;
> >         unsigned int is_krb5:1;
> >         unsigned int is_noauth:1;
> > +       unsigned int is_guest:1;
> >         uid_t sudo_uid;
> >  };
> >
> > @@ -1161,6 +1162,7 @@ parse_options(const char *data, struct parsed_mount_info *parsed_info)
> >                         parsed_info->got_user = 1;
> >                         parsed_info->got_password = 1;
> >                         parsed_info->got_password2 = 1;
> > +                       parsed_info->is_guest = 1;
> >                         goto nocopy;
> >                 case OPT_RO:
> >                         *filesys_flags |= MS_RDONLY;
> > @@ -2334,7 +2336,9 @@ mount_retry:
> >                 fprintf(stderr, "%s kernel mount options: %s",
> >                         thisprogram, options);
> >
> > -       if (parsed_info->got_password && !(parsed_info->is_krb5 || parsed_info->is_noauth)) {
> > +       if (parsed_info->got_password &&
> > +           !(parsed_info->is_krb5 || parsed_info->is_noauth ||
> > +             parsed_info->is_guest)) {
> >                 /*
> >                  * Commas have to be doubled, or else they will
> >                  * look like the parameter separator
> > @@ -2345,7 +2349,9 @@ mount_retry:
> >                         fprintf(stderr, ",pass=********");
> >         }
> >
> > -       if (parsed_info->got_password2 && !(parsed_info->is_krb5 || parsed_info->is_noauth)) {
> > +       if (parsed_info->got_password2 &&
> > +           !(parsed_info->is_krb5 || parsed_info->is_noauth ||
> > +             parsed_info->is_guest)) {
> >                 strlcat(options, ",password2=", options_size);
> >                 strlcat(options, parsed_info->password2, options_size);
> >                 if (parsed_info->verboseflag)



-- 
Thanks,

Steve

[-- Attachment #2: 0001-Fix-regression-in-mount.cifs-with-guest-mount-option.patch --]
[-- Type: text/x-patch, Size: 2281 bytes --]

From ac4d6e9cf69e1794dff061d62f727388a3b8b12b Mon Sep 17 00:00:00 2001
From: Meetakshi Setiya <msetiya@microsoft.com>
Date: Sat, 15 Mar 2025 06:49:13 +0000
Subject: [PATCH] Fix regression in mount.cifs with guest mount option

mount.cifs was erroneously passing the empty password and password2
strings to the linux smb client for "guest" mounts. Handling empty
password2 has not been implemented on the client yet, which ends up
failing the mount with "cifs: Bad value for password2".

This patch contains fixes for the mentioned scenario.

Signed-off-by: Meetakshi Setiya <msetiya@microsoft.com>
Reviewed-by: Paulo Alcantara (Red Hat) <pc@manguebit.com>
Reviewed-by: Bharath SM <bharathsm@microsoft.com>
Signed-off-by: Steve French <stfrench@microsoft.com>
---
 mount.cifs.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/mount.cifs.c b/mount.cifs.c
index 7605130..6edd96e 100644
--- a/mount.cifs.c
+++ b/mount.cifs.c
@@ -200,6 +200,7 @@ struct parsed_mount_info {
 	unsigned int got_domain:1;
 	unsigned int is_krb5:1;
 	unsigned int is_noauth:1;
+	unsigned int is_guest:1;
 	uid_t sudo_uid;
 };
 
@@ -1158,6 +1159,7 @@ parse_options(const char *data, struct parsed_mount_info *parsed_info)
 			*filesys_flags &= ~MS_NOEXEC;
 			goto nocopy;
 		case OPT_GUEST:
+			parsed_info->is_guest = 1;
 			parsed_info->got_user = 1;
 			parsed_info->got_password = 1;
 			parsed_info->got_password2 = 1;
@@ -2334,7 +2336,8 @@ mount_retry:
 		fprintf(stderr, "%s kernel mount options: %s",
 			thisprogram, options);
 
-	if (parsed_info->got_password && !(parsed_info->is_krb5 || parsed_info->is_noauth)) {
+	if (parsed_info->got_password &&
+		!(parsed_info->is_krb5 || parsed_info->is_noauth || parsed_info->is_guest)) {
 		/*
 		 * Commas have to be doubled, or else they will
 		 * look like the parameter separator
@@ -2345,7 +2348,8 @@ mount_retry:
 			fprintf(stderr, ",pass=********");
 	}
 
-	if (parsed_info->got_password2 && !(parsed_info->is_krb5 || parsed_info->is_noauth)) {
+	if (parsed_info->got_password2 &&
+		!(parsed_info->is_krb5 || parsed_info->is_noauth || parsed_info->is_guest)) {
 		strlcat(options, ",password2=", options_size);
 		strlcat(options, parsed_info->password2, options_size);
 		if (parsed_info->verboseflag)
-- 
2.43.0