| Message ID | 20250312135131.628756-1-pc@manguebit.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | smb: client: fix regression with guest option | expand |
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 >
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 <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.
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.
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 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)
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!
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)
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; } }
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(+)