cifs: fix parsing of password mount option
Subsystems (2)
PATCH v2 1/1 cifs: fix parsing of password mount option
The double delimiter check that allows a comma in the password parsing code is unconditional. We set "tmp_end" to the end of the string and we continue to check for double delimiter. In the case where the password doesn't contain a comma we end up setting tmp_end to NULL and eventually setting "options" to "end". This results in the premature termination of the options string and hence the values of UNCip and UNC are being set to NULL. This results in mount failure with "Connecting to DFS root not implemented yet" error.
This error is usually not noticable as we have password as the last option in the superblock mountdata. But when we call expand_dfs_referral() from cifs_mount() and try to compose mount options for the submount, the resulting mountdata will be of the form
",ver=1,user=foo,pass=bar,ip=x.x.x.x,unc=\\server\share"
and hence results in the above error. This bug has been seen with older NAS servers running Samba 3.0.24.
Fix this by moving the double delimiter check inside the conditional loop.
Changes since -v1
- removed the wrong strlen() micro optimization.
--- fs/cifs/connect.c | 32 +++++++++++++++++--------------- 1 files changed, 17 insertions(+), 15 deletions(-)
fs/cifs/connect.c +17 -15
@@ -1653,24 +1653,26 @@ cifs_parse_mount_options(const char *mountdata, const char *devname,* If yes, we have encountered a double deliminator * reset the NULL character to the deliminator */ -if (tmp_end < end && tmp_end[1 ] == delim) +if (tmp_end < end && tmp_end[1 ] == delim) { tmp_end[0 ] = delim; -/* Keep iterating until we get to a single deliminator -* OR the end -*/ -while ((tmp_end =strchr (tmp_end, delim)) !=NULL && - (tmp_end[1 ] == delim)) { - tmp_end = (char *) &tmp_end[2 ]; - } +/* Keep iterating until we get to a single +* deliminator OR the end +*/ +while ((tmp_end =strchr (tmp_end, delim)) + !=NULL && (tmp_end[1 ] == delim)) { + tmp_end = (char *) &tmp_end[2 ]; + } -/* Reset var options to point to next element */ -if (tmp_end) { - tmp_end[0 ] ='\0' ; - options = (char *) &tmp_end[1 ]; - }else -/* Reached the end of the mount option string */ - options = end; +/* Reset var options to point to next element */ +if (tmp_end) { + tmp_end[0 ] ='\0' ; + options = (char *) &tmp_end[1 ]; + }else +/* Reached the end of the mount option +* string */ + options = end; + }/* Now build new password string */ temp_len =strlen (value);
Sachin Prabhu
Acked-byQuoted 66 lines · Suresh Jayaraman
The double delimiter check that allows a comma in the password parsing code is unconditional. We set "
tmp_end" to the end of the string and we continue to check for double delimiter. In the case where the password doesn't contain a comma we end up settingtmp_endto NULL and eventually setting "options" to "end". This results in the premature termination of the options string and hence the values of UNCip and UNC are being set to NULL. This results in mount failure with "Connecting to DFS root not implemented yet" error.This error is usually not noticable as we have password as the last option in the superblock mountdata. But when we call
expand_dfs_referral()fromcifs_mount()and try to compose mount options for the submount, the resulting mountdata will be of the form",ver=1,user=foo,pass=bar,ip=x.x.x.x,unc=\\server\share"and hence results in the above error. This bug has been seen with older NAS servers running Samba 3.0.24.
Fix this by moving the double delimiter check inside the conditional loop.
Changes since -v1
- removed the wrong
strlen()micro optimization.Signed-off-by Suresh Jayaraman <sjayaraman-IBi9RG/[email protected]>Cc Sachin Prabhu <[email protected]>--- fs/cifs/connect.c | 32 +++++++++++++++++--------------- 1 files changed, 17 insertions(+), 15 deletions(-)fs/cifs/connect.c +10
-15@@ -1653,24 +1653,26 @@ cifs_parse_mount_options(const char *mountdata, const char *devname,* If yes, we have encountered a double deliminator * reset the NULL character to the deliminator */ -if (tmp_end < end && tmp_end[1 ] == delim) +if (tmp_end < end && tmp_end[1 ] == delim) { tmp_end[0 ] = delim; -/* Keep iterating until we get to a single deliminator -* OR the end -*/ -while ((tmp_end =strchr (tmp_end, delim)) !=NULL && - (tmp_end[1 ] == delim)) { - tmp_end = (char *) &tmp_end[2 ]; - } +/* Keep iterating until we get to a single +* deliminator OR the end +*/ +while ((tmp_end =strchr (tmp_end, delim)) + !=NULL && (tmp_end[1 ] == delim)) { + tmp_end = (char *) &tmp_end[2 ]; + } -/* Reset var options to point to next element */ -if (tmp_end) { - tmp_end[0 ] ='\0' ; - options = (char *) &tmp_end[1 ]; - }else -/* Reached the end of the mount option string */ - options = end; +/* Reset var options to point to next element */ +if (tmp_end) {
fs/cifs/connect.c cifs_parse_mount_options(const char *mountdata, const char *devname, · line 1678
Quoted 10 lines
fs/cifs/connect.c +7
-0+ tmp_end[0 ] ='\0' ; + options = (char *) &tmp_end[1 ]; + }else +/* Reached the end of the mount option +* string */ + options = end; + }/* Now build new password string */ temp_len =strlen (value);
Another way would have been to leave the earlier optimisation in place but simply reset temp_len within the if condition. This means the second strlen was called only in case we had encountered a double delimiter in the password.
In any case, this code is called only once at mount time and hence the optimisation is not terribly important in this case.
Quoted 4 lines
--
To unsubscribe from this list: send the line "unsubscribe linux-cifs" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html