[PATCH] cifs: extend the buffer length enought for sprintf() using

[Chen Gang]

For cifs_set_cifscreds() in "fs/cifs/connect.c", 'desc' buffer length
is 'CIFSCREDS_DESC_SIZE' (56 is less than 256), and 'ses->domainName'
length may be "255 + '\0'".

The related sprintf() may cause memory overflow, so need extend related
buffer enough to hold all things.

It is also necessary to be sure of 'ses->domainName' must be less than
256, and define the related macro instead of hard code number '256'.

Signed-off-by: Chen Gang <gang.chen-bOixZGp5f+***@public.gmane.org>
---
fs/cifs/cifsencrypt.c | 2 +-
fs/cifs/cifsglob.h | 1 +
fs/cifs/connect.c | 7 ++++---
fs/cifs/sess.c | 6 +++---
4 files changed, 9 insertions(+), 7 deletions(-)

diff --git a/fs/cifs/cifsencrypt.c b/fs/cifs/cifsencrypt.c
index 45e57cc..ce2d331 100644
--- a/fs/cifs/cifsencrypt.c
+++ b/fs/cifs/cifsencrypt.c
@@ -421,7 +421,7 @@ find_domain_name(struct cifs_ses *ses, const struct nls_table *nls_cp)
if (blobptr + attrsize > blobend)
break;
if (type == NTLMSSP_AV_NB_DOMAIN_NAME) {
- if (!attrsize)
+ if (!attrsize || attrsize >= MAX_CIF_DOMAINNAME)
break;
if (!ses->domainName) {
ses->domainName =
diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
index 33f17ed..88280e0 100644
--- a/fs/cifs/cifsglob.h
+++ b/fs/cifs/cifsglob.h
@@ -396,6 +396,7 @@ struct smb_version_values {

#define HEADER_SIZE(server) (server->vals->header_size)
#define MAX_HEADER_SIZE(server) (server->vals->max_header_size)
+#define MAX_CIF_DOMAINNAME 256 /* 255 + '\0' */

struct smb_vol {
char *username;
diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
index fa68813..ed6bf20 100644
--- a/fs/cifs/connect.c
+++ b/fs/cifs/connect.c
@@ -1675,7 +1675,8 @@ cifs_parse_mount_options(const char *mountdata, const char *devname,
if (string == NULL)
goto out_nomem;

- if (strnlen(string, 256) == 256) {
+ if (strnlen(string, MAX_CIF_DOMAINNAME)
+ == MAX_CIF_DOMAINNAME) {
printk(KERN_WARNING "CIFS: domain name too"
" long\n");
goto cifs_parse_mount_err;
@@ -2276,8 +2277,8 @@ cifs_put_smb_ses(struct cifs_ses *ses)

#ifdef CONFIG_KEYS

-/* strlen("cifs:a:") + INET6_ADDRSTRLEN + 1 */
-#define CIFSCREDS_DESC_SIZE (7 + INET6_ADDRSTRLEN + 1)
+/* strlen("cifs:a:") + MAX_CIF_DOMAINNAME + 1 */
+#define CIFSCREDS_DESC_SIZE (7 + MAX_CIF_DOMAINNAME + 1)

/* Populate username and pw fields from keyring if possible */
static int
diff --git a/fs/cifs/sess.c b/fs/cifs/sess.c
index 79358e3..106a575 100644
--- a/fs/cifs/sess.c
+++ b/fs/cifs/sess.c
@@ -197,7 +197,7 @@ static void unicode_domain_string(char **pbcc_area, struct cifs_ses *ses,
bytes_ret = 0;
} else
bytes_ret = cifs_strtoUTF16((__le16 *) bcc_ptr, ses->domainName,
- 256, nls_cp);
+ MAX_CIF_DOMAINNAME, nls_cp);
bcc_ptr += 2 * bytes_ret;
bcc_ptr += 2; /* account for null terminator */

@@ -255,8 +255,8 @@ static void ascii_ssetup_strings(char **pbcc_area, struct cifs_ses *ses,

/* copy domain */
if (ses->domainName != NULL) {
- strncpy(bcc_ptr, ses->domainName, 256);
- bcc_ptr += strnlen(ses->domainName, 256);
+ strncpy(bcc_ptr, ses->domainName, MAX_CIF_DOMAINNAME);
+ bcc_ptr += strnlen(ses->domainName, MAX_CIF_DOMAINNAME);
} /* else we will send a null domain name
so the server will default to its own domain */
*bcc_ptr = 0;

[Scott Lovenberg]

Post by Chen Gang
For cifs_set_cifscreds() in "fs/cifs/connect.c", 'desc' buffer length
is 'CIFSCREDS_DESC_SIZE' (56 is less than 256), and 'ses->domainName'
length may be "255 + '\0'".
The related sprintf() may cause memory overflow, so need extend related
buffer enough to hold all things.
It is also necessary to be sure of 'ses->domainName' must be less than
256, and define the related macro instead of hard code number '256'.
---
fs/cifs/cifsencrypt.c | 2 +-
fs/cifs/cifsglob.h | 1 +
fs/cifs/connect.c | 7 ++++---
fs/cifs/sess.c | 6 +++---
4 files changed, 9 insertions(+), 7 deletions(-)
diff --git a/fs/cifs/cifsencrypt.c b/fs/cifs/cifsencrypt.c
index 45e57cc..ce2d331 100644
--- a/fs/cifs/cifsencrypt.c
+++ b/fs/cifs/cifsencrypt.c
@@ -421,7 +421,7 @@ find_domain_name(struct cifs_ses *ses, const struct nls_table *nls_cp)
if (blobptr + attrsize > blobend)
break;
if (type == NTLMSSP_AV_NB_DOMAIN_NAME) {
- if (!attrsize)
+ if (!attrsize || attrsize >= MAX_CIF_DOMAINNAME)
break;
if (!ses->domainName) {
ses->domainName =
diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
index 33f17ed..88280e0 100644
--- a/fs/cifs/cifsglob.h
+++ b/fs/cifs/cifsglob.h
@@ -396,6 +396,7 @@ struct smb_version_values {
#define HEADER_SIZE(server) (server->vals->header_size)
#define MAX_HEADER_SIZE(server) (server->vals->max_header_size)
+#define MAX_CIF_DOMAINNAME 256 /* 255 + '\0' */
struct smb_vol {
char *username;
diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
index fa68813..ed6bf20 100644
--- a/fs/cifs/connect.c
+++ b/fs/cifs/connect.c
@@ -1675,7 +1675,8 @@ cifs_parse_mount_options(const char *mountdata, const char *devname,
if (string == NULL)
goto out_nomem;
- if (strnlen(string, 256) == 256) {
+ if (strnlen(string, MAX_CIF_DOMAINNAME)
+ == MAX_CIF_DOMAINNAME) {
printk(KERN_WARNING "CIFS: domain name too"
" long\n");
goto cifs_parse_mount_err;
@@ -2276,8 +2277,8 @@ cifs_put_smb_ses(struct cifs_ses *ses)
#ifdef CONFIG_KEYS
-/* strlen("cifs:a:") + INET6_ADDRSTRLEN + 1 */
-#define CIFSCREDS_DESC_SIZE (7 + INET6_ADDRSTRLEN + 1)
+/* strlen("cifs:a:") + MAX_CIF_DOMAINNAME + 1 */
+#define CIFSCREDS_DESC_SIZE (7 + MAX_CIF_DOMAINNAME + 1)
/* Populate username and pw fields from keyring if possible */
static int
diff --git a/fs/cifs/sess.c b/fs/cifs/sess.c
index 79358e3..106a575 100644
--- a/fs/cifs/sess.c
+++ b/fs/cifs/sess.c
@@ -197,7 +197,7 @@ static void unicode_domain_string(char **pbcc_area, struct cifs_ses *ses,
bytes_ret = 0;
} else
bytes_ret = cifs_strtoUTF16((__le16 *) bcc_ptr, ses->domainName,
- 256, nls_cp);
+ MAX_CIF_DOMAINNAME, nls_cp);
bcc_ptr += 2 * bytes_ret;
bcc_ptr += 2; /* account for null terminator */
@@ -255,8 +255,8 @@ static void ascii_ssetup_strings(char **pbcc_area, struct cifs_ses *ses,
/* copy domain */
if (ses->domainName != NULL) {
- strncpy(bcc_ptr, ses->domainName, 256);
- bcc_ptr += strnlen(ses->domainName, 256);
+ strncpy(bcc_ptr, ses->domainName, MAX_CIF_DOMAINNAME);
+ bcc_ptr += strnlen(ses->domainName, MAX_CIF_DOMAINNAME);
} /* else we will send a null domain name
so the server will default to its own domain */
*bcc_ptr = 0;
--
1.7.7.6
--
To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
More majordomo info at http://vger.kernel.org/majordomo-info.html

If this is correct for the domain size, MAX_DOMAIN_SIZE in the
cifs-utils mount helper (mount.cifs.c) has to be changed. It is
currently 64 + null terminator. I can open a bug and patch it if this
is correct.

CC'ing Jeff Layton since he maintains the cifs-utils package.

--
Peace and Blessings,
-Scott.

[Steve French]

On Tue, Jul 16, 2013 at 8:58 PM, Scott Lovenberg

Post by Scott Lovenberg

Post by Chen Gang
For cifs_set_cifscreds() in "fs/cifs/connect.c", 'desc' buffer length
is 'CIFSCREDS_DESC_SIZE' (56 is less than 256), and 'ses->domainName'
length may be "255 + '\0'".
The related sprintf() may cause memory overflow, so need extend related
buffer enough to hold all things.
It is also necessary to be sure of 'ses->domainName' must be less than
256, and define the related macro instead of hard code number '256'.
---
fs/cifs/cifsencrypt.c | 2 +-
fs/cifs/cifsglob.h | 1 +
fs/cifs/connect.c | 7 ++++---
fs/cifs/sess.c | 6 +++---
4 files changed, 9 insertions(+), 7 deletions(-)
diff --git a/fs/cifs/cifsencrypt.c b/fs/cifs/cifsencrypt.c
index 45e57cc..ce2d331 100644
--- a/fs/cifs/cifsencrypt.c
+++ b/fs/cifs/cifsencrypt.c
@@ -421,7 +421,7 @@ find_domain_name(struct cifs_ses *ses, const struct nls_table *nls_cp)
if (blobptr + attrsize > blobend)
break;
if (type == NTLMSSP_AV_NB_DOMAIN_NAME) {
- if (!attrsize)
+ if (!attrsize || attrsize >= MAX_CIF_DOMAINNAME)
break;
if (!ses->domainName) {
ses->domainName =
diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
index 33f17ed..88280e0 100644
--- a/fs/cifs/cifsglob.h
+++ b/fs/cifs/cifsglob.h
@@ -396,6 +396,7 @@ struct smb_version_values {
#define HEADER_SIZE(server) (server->vals->header_size)
#define MAX_HEADER_SIZE(server) (server->vals->max_header_size)
+#define MAX_CIF_DOMAINNAME 256 /* 255 + '\0' */
struct smb_vol {
char *username;
diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
index fa68813..ed6bf20 100644
--- a/fs/cifs/connect.c
+++ b/fs/cifs/connect.c
@@ -1675,7 +1675,8 @@ cifs_parse_mount_options(const char *mountdata, const char *devname,
if (string == NULL)
goto out_nomem;
- if (strnlen(string, 256) == 256) {
+ if (strnlen(string, MAX_CIF_DOMAINNAME)
+ == MAX_CIF_DOMAINNAME) {
printk(KERN_WARNING "CIFS: domain name too"
" long\n");
goto cifs_parse_mount_err;
@@ -2276,8 +2277,8 @@ cifs_put_smb_ses(struct cifs_ses *ses)
#ifdef CONFIG_KEYS
-/* strlen("cifs:a:") + INET6_ADDRSTRLEN + 1 */
-#define CIFSCREDS_DESC_SIZE (7 + INET6_ADDRSTRLEN + 1)
+/* strlen("cifs:a:") + MAX_CIF_DOMAINNAME + 1 */
+#define CIFSCREDS_DESC_SIZE (7 + MAX_CIF_DOMAINNAME + 1)
/* Populate username and pw fields from keyring if possible */
static int
diff --git a/fs/cifs/sess.c b/fs/cifs/sess.c
index 79358e3..106a575 100644
--- a/fs/cifs/sess.c
+++ b/fs/cifs/sess.c
@@ -197,7 +197,7 @@ static void unicode_domain_string(char **pbcc_area, struct cifs_ses *ses,
bytes_ret = 0;
} else
bytes_ret = cifs_strtoUTF16((__le16 *) bcc_ptr, ses->domainName,
- 256, nls_cp);
+ MAX_CIF_DOMAINNAME, nls_cp);
bcc_ptr += 2 * bytes_ret;
bcc_ptr += 2; /* account for null terminator */
@@ -255,8 +255,8 @@ static void ascii_ssetup_strings(char **pbcc_area, struct cifs_ses *ses,
/* copy domain */
if (ses->domainName != NULL) {
- strncpy(bcc_ptr, ses->domainName, 256);
- bcc_ptr += strnlen(ses->domainName, 256);
+ strncpy(bcc_ptr, ses->domainName, MAX_CIF_DOMAINNAME);
+ bcc_ptr += strnlen(ses->domainName, MAX_CIF_DOMAINNAME);
} /* else we will send a null domain name
so the server will default to its own domain */
*bcc_ptr = 0;
--
1.7.7.6
--
To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
More majordomo info at http://vger.kernel.org/majordomo-info.html

If this is correct for the domain size, MAX_DOMAIN_SIZE in the
cifs-utils mount helper (mount.cifs.c) has to be changed. It is
currently 64 + null terminator. I can open a bug and patch it if this
is correct.
CC'ing Jeff Layton since he maintains the cifs-utils package.
--
Peace and Blessings,
-Scott.

http://support.microsoft.com/kb/909264 indicates that (at least for
Microsoft) domain names can be considerably longer than 64 bytes, so
this patch seems reasonable. Merged into cifs-2.6.git - if anyone
objects or wants to add ack/reviewed let me know.


"The maximum length of ... the fully qualified domain name (FQDN) is
63 octets per label and 255 bytes per FQDN. This maximum includes 254
bytes for the FQDN and one byte for the ending dot."

[Chen Gang]

Post by Steve French
On Tue, Jul 16, 2013 at 8:58 PM, Scott Lovenberg

Post by Scott Lovenberg

Post by Chen Gang
For cifs_set_cifscreds() in "fs/cifs/connect.c", 'desc' buffer length
is 'CIFSCREDS_DESC_SIZE' (56 is less than 256), and 'ses->domainName'
length may be "255 + '\0'".
The related sprintf() may cause memory overflow, so need extend related
buffer enough to hold all things.
It is also necessary to be sure of 'ses->domainName' must be less than
256, and define the related macro instead of hard code number '256'.
---
fs/cifs/cifsencrypt.c | 2 +-
fs/cifs/cifsglob.h | 1 +
fs/cifs/connect.c | 7 ++++---
fs/cifs/sess.c | 6 +++---
4 files changed, 9 insertions(+), 7 deletions(-)
diff --git a/fs/cifs/cifsencrypt.c b/fs/cifs/cifsencrypt.c
index 45e57cc..ce2d331 100644
--- a/fs/cifs/cifsencrypt.c
+++ b/fs/cifs/cifsencrypt.c
@@ -421,7 +421,7 @@ find_domain_name(struct cifs_ses *ses, const struct nls_table *nls_cp)
if (blobptr + attrsize > blobend)
break;
if (type == NTLMSSP_AV_NB_DOMAIN_NAME) {
- if (!attrsize)
+ if (!attrsize || attrsize >= MAX_CIF_DOMAINNAME)
break;
if (!ses->domainName) {
ses->domainName =
diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
index 33f17ed..88280e0 100644
--- a/fs/cifs/cifsglob.h
+++ b/fs/cifs/cifsglob.h
@@ -396,6 +396,7 @@ struct smb_version_values {
#define HEADER_SIZE(server) (server->vals->header_size)
#define MAX_HEADER_SIZE(server) (server->vals->max_header_size)
+#define MAX_CIF_DOMAINNAME 256 /* 255 + '\0' */
struct smb_vol {
char *username;
diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
index fa68813..ed6bf20 100644
--- a/fs/cifs/connect.c
+++ b/fs/cifs/connect.c
@@ -1675,7 +1675,8 @@ cifs_parse_mount_options(const char *mountdata, const char *devname,
if (string == NULL)
goto out_nomem;
- if (strnlen(string, 256) == 256) {
+ if (strnlen(string, MAX_CIF_DOMAINNAME)
+ == MAX_CIF_DOMAINNAME) {
printk(KERN_WARNING "CIFS: domain name too"
" long\n");
goto cifs_parse_mount_err;
@@ -2276,8 +2277,8 @@ cifs_put_smb_ses(struct cifs_ses *ses)
#ifdef CONFIG_KEYS
-/* strlen("cifs:a:") + INET6_ADDRSTRLEN + 1 */
-#define CIFSCREDS_DESC_SIZE (7 + INET6_ADDRSTRLEN + 1)
+/* strlen("cifs:a:") + MAX_CIF_DOMAINNAME + 1 */
+#define CIFSCREDS_DESC_SIZE (7 + MAX_CIF_DOMAINNAME + 1)
/* Populate username and pw fields from keyring if possible */
static int
diff --git a/fs/cifs/sess.c b/fs/cifs/sess.c
index 79358e3..106a575 100644
--- a/fs/cifs/sess.c
+++ b/fs/cifs/sess.c
@@ -197,7 +197,7 @@ static void unicode_domain_string(char **pbcc_area, struct cifs_ses *ses,
bytes_ret = 0;
} else
bytes_ret = cifs_strtoUTF16((__le16 *) bcc_ptr, ses->domainName,
- 256, nls_cp);
+ MAX_CIF_DOMAINNAME, nls_cp);
bcc_ptr += 2 * bytes_ret;
bcc_ptr += 2; /* account for null terminator */
@@ -255,8 +255,8 @@ static void ascii_ssetup_strings(char **pbcc_area, struct cifs_ses *ses,
/* copy domain */
if (ses->domainName != NULL) {
- strncpy(bcc_ptr, ses->domainName, 256);
- bcc_ptr += strnlen(ses->domainName, 256);
+ strncpy(bcc_ptr, ses->domainName, MAX_CIF_DOMAINNAME);
+ bcc_ptr += strnlen(ses->domainName, MAX_CIF_DOMAINNAME);
} /* else we will send a null domain name
so the server will default to its own domain */
*bcc_ptr = 0;
--
1.7.7.6
--
To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
More majordomo info at http://vger.kernel.org/majordomo-info.html

If this is correct for the domain size, MAX_DOMAIN_SIZE in the
cifs-utils mount helper (mount.cifs.c) has to be changed. It is
currently 64 + null terminator. I can open a bug and patch it if this
is correct.
CC'ing Jeff Layton since he maintains the cifs-utils package.
--
Peace and Blessings,
-Scott.

http://support.microsoft.com/kb/909264 indicates that (at least for
Microsoft) domain names can be considerably longer than 64 bytes, so
this patch seems reasonable. Merged into cifs-2.6.git - if anyone
objects or wants to add ack/reviewed let me know.
"The maximum length of ... the fully qualified domain name (FQDN) is
63 octets per label and 255 bytes per FQDN. This maximum includes 254
bytes for the FQDN and one byte for the ending dot."

Thank you for your valuable information.

[Scott Lovenberg]

Post by Steve French

Post by Scott Lovenberg
If this is correct for the domain size, MAX_DOMAIN_SIZE in the
cifs-utils mount helper (mount.cifs.c) has to be changed. It is
currently 64 + null terminator. I can open a bug and patch it if this
is correct.
CC'ing Jeff Layton since he maintains the cifs-utils package.
--
Peace and Blessings,
-Scott.

http://support.microsoft.com/kb/909264 indicates that (at least for
Microsoft) domain names can be considerably longer than 64 bytes, so
this patch seems reasonable. Merged into cifs-2.6.git - if anyone
objects or wants to add ack/reviewed let me know.
"The maximum length of ... the fully qualified domain name (FQDN) is
63 octets per label and 255 bytes per FQDN. This maximum includes 254
bytes for the FQDN and one byte for the ending dot."
--
Thanks,
Steve

Thanks, Steve. I'll patch this tonight.


--
Peace and Blessings,
-Scott.

[Chen Gang]

Post by Scott Lovenberg

Post by Steve French

Post by Scott Lovenberg
If this is correct for the domain size, MAX_DOMAIN_SIZE in the
cifs-utils mount helper (mount.cifs.c) has to be changed. It is
currently 64 + null terminator. I can open a bug and patch it if this
is correct.
CC'ing Jeff Layton since he maintains the cifs-utils package.
--
Peace and Blessings,
-Scott.

http://support.microsoft.com/kb/909264 indicates that (at least for
Microsoft) domain names can be considerably longer than 64 bytes, so
this patch seems reasonable. Merged into cifs-2.6.git - if anyone
objects or wants to add ack/reviewed let me know.
"The maximum length of ... the fully qualified domain name (FQDN) is
63 octets per label and 255 bytes per FQDN. This maximum includes 254
bytes for the FQDN and one byte for the ending dot."
--
Thanks,
Steve

Thanks, Steve. I'll patch this tonight.

Firstly, thank you for your work.

Hmm... could you please wait for 1-2 days so can let this patch given
more checking by another contributors ?

If possible, after finish discussing, I should/will send patch v2 for it.

Is it OK ?

Post by Scott Lovenberg
--
Peace and Blessings,
-Scott.

Thanks.

[Scott Lovenberg]

Post by Chen Gang

Post by Scott Lovenberg

Post by Steve French

Post by Scott Lovenberg
If this is correct for the domain size, MAX_DOMAIN_SIZE in the
cifs-utils mount helper (mount.cifs.c) has to be changed. It is
currently 64 + null terminator. I can open a bug and patch it if this
is correct.
CC'ing Jeff Layton since he maintains the cifs-utils package.
--
Peace and Blessings,
-Scott.

http://support.microsoft.com/kb/909264 indicates that (at least for
Microsoft) domain names can be considerably longer than 64 bytes, so
this patch seems reasonable. Merged into cifs-2.6.git - if anyone
objects or wants to add ack/reviewed let me know.
"The maximum length of ... the fully qualified domain name (FQDN) is
63 octets per label and 255 bytes per FQDN. This maximum includes 254
bytes for the FQDN and one byte for the ending dot."
--
Thanks,
Steve

Thanks, Steve. I'll patch this tonight.

Firstly, thank you for your work.
Hmm... could you please wait for 1-2 days so can let this patch given
more checking by another contributors ?
If possible, after finish discussing, I should/will send patch v2 for it.
Is it OK ?
Thanks.
--
Chen Gang

Thank you for your work. :)
That's completely reasonable. I'll submit in a day or two.

--
Peace and Blessings,
-Scott.

[Chen Gang]

For cifs_set_cifscreds() in "fs/cifs/connect.c", 'desc' buffer length
is 'CIFSCREDS_DESC_SIZE' (56 is less than 256), and 'ses->domainName'
length may be "255 + '\0'".

The related sprintf() may cause memory overflow, so need extend related
buffer enough to hold all things.

It is also necessary to be sure of 'ses->domainName' must be less than
256, and define the related macro instead of hard code number '256'.

Signed-off-by: Chen Gang <gang.chen-bOixZGp5f+***@public.gmane.org>
---
fs/cifs/cifsencrypt.c | 2 +-
fs/cifs/cifsglob.h | 1 +
fs/cifs/connect.c | 7 ++++---
fs/cifs/sess.c | 6 +++---
4 files changed, 9 insertions(+), 7 deletions(-)

diff --git a/fs/cifs/cifsencrypt.c b/fs/cifs/cifsencrypt.c
index 45e57cc..bb84d7c 100644
--- a/fs/cifs/cifsencrypt.c
+++ b/fs/cifs/cifsencrypt.c
@@ -421,7 +421,7 @@ find_domain_name(struct cifs_ses *ses, const struct nls_table *nls_cp)
if (blobptr + attrsize > blobend)
break;
if (type == NTLMSSP_AV_NB_DOMAIN_NAME) {
- if (!attrsize)
+ if (!attrsize || attrsize >= MAX_DOMAINNAME_SIZE)
break;
if (!ses->domainName) {
ses->domainName =
diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
index 33f17ed..64bb4c5 100644
--- a/fs/cifs/cifsglob.h
+++ b/fs/cifs/cifsglob.h
@@ -44,6 +44,7 @@
#define MAX_TREE_SIZE (2 + MAX_SERVER_SIZE + 1 + MAX_SHARE_SIZE + 1)
#define MAX_SERVER_SIZE 15
#define MAX_SHARE_SIZE 80
+#define MAX_DOMAINNAME_SIZE 256 /* maximum fully qualified domain name (FQDN) */
#define MAX_USERNAME_SIZE 256 /* reasonable maximum for current servers */
#define MAX_PASSWORD_SIZE 512 /* max for windows seems to be 256 wide chars */

diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
index fa68813..3f9dcf7 100644
--- a/fs/cifs/connect.c
+++ b/fs/cifs/connect.c
@@ -1675,7 +1675,8 @@ cifs_parse_mount_options(const char *mountdata, const char *devname,
if (string == NULL)
goto out_nomem;

- if (strnlen(string, 256) == 256) {
+ if (strnlen(string, MAX_DOMAINNAME_SIZE)
+ == MAX_DOMAINNAME_SIZE) {
printk(KERN_WARNING "CIFS: domain name too"
" long\n");
goto cifs_parse_mount_err;
@@ -2276,8 +2277,8 @@ cifs_put_smb_ses(struct cifs_ses *ses)

#ifdef CONFIG_KEYS

-/* strlen("cifs:a:") + INET6_ADDRSTRLEN + 1 */
-#define CIFSCREDS_DESC_SIZE (7 + INET6_ADDRSTRLEN + 1)
+/* strlen("cifs:a:") + MAX_DOMAINNAME_SIZE + 1 */
+#define CIFSCREDS_DESC_SIZE (7 + MAX_DOMAINNAME_SIZE + 1)

/* Populate username and pw fields from keyring if possible */
static int
diff --git a/fs/cifs/sess.c b/fs/cifs/sess.c
index 79358e3..57b1537 100644
--- a/fs/cifs/sess.c
+++ b/fs/cifs/sess.c
@@ -197,7 +197,7 @@ static void unicode_domain_string(char **pbcc_area, struct cifs_ses *ses,
bytes_ret = 0;
} else
bytes_ret = cifs_strtoUTF16((__le16 *) bcc_ptr, ses->domainName,
- 256, nls_cp);
+ MAX_DOMAINNAME_SIZE, nls_cp);
bcc_ptr += 2 * bytes_ret;
bcc_ptr += 2; /* account for null terminator */

@@ -255,8 +255,8 @@ static void ascii_ssetup_strings(char **pbcc_area, struct cifs_ses *ses,

/* copy domain */
if (ses->domainName != NULL) {
- strncpy(bcc_ptr, ses->domainName, 256);
- bcc_ptr += strnlen(ses->domainName, 256);
+ strncpy(bcc_ptr, ses->domainName, MAX_DOMAINNAME_SIZE);
+ bcc_ptr += strnlen(ses->domainName, MAX_DOMAINNAME_SIZE);
} /* else we will send a null domain name
so the server will default to its own domain */
*bcc_ptr = 0;

[Jeff Layton]

On Fri, 19 Jul 2013 09:01:36 +0800

Post by Chen Gang
For cifs_set_cifscreds() in "fs/cifs/connect.c", 'desc' buffer length
is 'CIFSCREDS_DESC_SIZE' (56 is less than 256), and 'ses->domainName'
length may be "255 + '\0'".
The related sprintf() may cause memory overflow, so need extend related
buffer enough to hold all things.
It is also necessary to be sure of 'ses->domainName' must be less than
256, and define the related macro instead of hard code number '256'.
---
fs/cifs/cifsencrypt.c | 2 +-
fs/cifs/cifsglob.h | 1 +
fs/cifs/connect.c | 7 ++++---
fs/cifs/sess.c | 6 +++---
4 files changed, 9 insertions(+), 7 deletions(-)
diff --git a/fs/cifs/cifsencrypt.c b/fs/cifs/cifsencrypt.c
index 45e57cc..bb84d7c 100644
--- a/fs/cifs/cifsencrypt.c
+++ b/fs/cifs/cifsencrypt.c
@@ -421,7 +421,7 @@ find_domain_name(struct cifs_ses *ses, const struct nls_table *nls_cp)
if (blobptr + attrsize > blobend)
break;
if (type == NTLMSSP_AV_NB_DOMAIN_NAME) {
- if (!attrsize)
+ if (!attrsize || attrsize >= MAX_DOMAINNAME_SIZE)
break;
if (!ses->domainName) {
ses->domainName =
diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
index 33f17ed..64bb4c5 100644
--- a/fs/cifs/cifsglob.h
+++ b/fs/cifs/cifsglob.h
@@ -44,6 +44,7 @@
#define MAX_TREE_SIZE (2 + MAX_SERVER_SIZE + 1 + MAX_SHARE_SIZE + 1)
#define MAX_SERVER_SIZE 15
#define MAX_SHARE_SIZE 80
+#define MAX_DOMAINNAME_SIZE 256 /* maximum fully qualified domain name (FQDN) */
#define MAX_USERNAME_SIZE 256 /* reasonable maximum for current servers */
#define MAX_PASSWORD_SIZE 512 /* max for windows seems to be 256 wide chars */
diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
index fa68813..3f9dcf7 100644
--- a/fs/cifs/connect.c
+++ b/fs/cifs/connect.c
@@ -1675,7 +1675,8 @@ cifs_parse_mount_options(const char *mountdata, const char *devname,
if (string == NULL)
goto out_nomem;
- if (strnlen(string, 256) == 256) {
+ if (strnlen(string, MAX_DOMAINNAME_SIZE)
+ == MAX_DOMAINNAME_SIZE) {
printk(KERN_WARNING "CIFS: domain name too"
" long\n");
goto cifs_parse_mount_err;
@@ -2276,8 +2277,8 @@ cifs_put_smb_ses(struct cifs_ses *ses)
#ifdef CONFIG_KEYS
-/* strlen("cifs:a:") + INET6_ADDRSTRLEN + 1 */
-#define CIFSCREDS_DESC_SIZE (7 + INET6_ADDRSTRLEN + 1)
+/* strlen("cifs:a:") + MAX_DOMAINNAME_SIZE + 1 */
+#define CIFSCREDS_DESC_SIZE (7 + MAX_DOMAINNAME_SIZE + 1)
/* Populate username and pw fields from keyring if possible */
static int
diff --git a/fs/cifs/sess.c b/fs/cifs/sess.c
index 79358e3..57b1537 100644
--- a/fs/cifs/sess.c
+++ b/fs/cifs/sess.c
@@ -197,7 +197,7 @@ static void unicode_domain_string(char **pbcc_area, struct cifs_ses *ses,
bytes_ret = 0;
} else
bytes_ret = cifs_strtoUTF16((__le16 *) bcc_ptr, ses->domainName,
- 256, nls_cp);
+ MAX_DOMAINNAME_SIZE, nls_cp);
bcc_ptr += 2 * bytes_ret;
bcc_ptr += 2; /* account for null terminator */
@@ -255,8 +255,8 @@ static void ascii_ssetup_strings(char **pbcc_area, struct cifs_ses *ses,
/* copy domain */
if (ses->domainName != NULL) {
- strncpy(bcc_ptr, ses->domainName, 256);
- bcc_ptr += strnlen(ses->domainName, 256);
+ strncpy(bcc_ptr, ses->domainName, MAX_DOMAINNAME_SIZE);
+ bcc_ptr += strnlen(ses->domainName, MAX_DOMAINNAME_SIZE);
} /* else we will send a null domain name
so the server will default to its own domain */
*bcc_ptr = 0;

Looks good...

Reviewed-by: Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+***@public.gmane.org>

[Steve French]

merged into cifs-2.6.git

Post by Jeff Layton
On Fri, 19 Jul 2013 09:01:36 +0800

Post by Chen Gang
For cifs_set_cifscreds() in "fs/cifs/connect.c", 'desc' buffer length
is 'CIFSCREDS_DESC_SIZE' (56 is less than 256), and 'ses->domainName'
length may be "255 + '\0'".
The related sprintf() may cause memory overflow, so need extend related
buffer enough to hold all things.
It is also necessary to be sure of 'ses->domainName' must be less than
256, and define the related macro instead of hard code number '256'.
---
fs/cifs/cifsencrypt.c | 2 +-
fs/cifs/cifsglob.h | 1 +
fs/cifs/connect.c | 7 ++++---
fs/cifs/sess.c | 6 +++---
4 files changed, 9 insertions(+), 7 deletions(-)
diff --git a/fs/cifs/cifsencrypt.c b/fs/cifs/cifsencrypt.c
index 45e57cc..bb84d7c 100644
--- a/fs/cifs/cifsencrypt.c
+++ b/fs/cifs/cifsencrypt.c
@@ -421,7 +421,7 @@ find_domain_name(struct cifs_ses *ses, const struct nls_table *nls_cp)
if (blobptr + attrsize > blobend)
break;
if (type == NTLMSSP_AV_NB_DOMAIN_NAME) {
- if (!attrsize)
+ if (!attrsize || attrsize >= MAX_DOMAINNAME_SIZE)
break;
if (!ses->domainName) {
ses->domainName =
diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
index 33f17ed..64bb4c5 100644
--- a/fs/cifs/cifsglob.h
+++ b/fs/cifs/cifsglob.h
@@ -44,6 +44,7 @@
#define MAX_TREE_SIZE (2 + MAX_SERVER_SIZE + 1 + MAX_SHARE_SIZE + 1)
#define MAX_SERVER_SIZE 15
#define MAX_SHARE_SIZE 80
+#define MAX_DOMAINNAME_SIZE 256 /* maximum fully qualified domain name (FQDN) */
#define MAX_USERNAME_SIZE 256 /* reasonable maximum for current servers */
#define MAX_PASSWORD_SIZE 512 /* max for windows seems to be 256 wide chars */
diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
index fa68813..3f9dcf7 100644
--- a/fs/cifs/connect.c
+++ b/fs/cifs/connect.c
@@ -1675,7 +1675,8 @@ cifs_parse_mount_options(const char *mountdata, const char *devname,
if (string == NULL)
goto out_nomem;
- if (strnlen(string, 256) == 256) {
+ if (strnlen(string, MAX_DOMAINNAME_SIZE)
+ == MAX_DOMAINNAME_SIZE) {
printk(KERN_WARNING "CIFS: domain name too"
" long\n");
goto cifs_parse_mount_err;
@@ -2276,8 +2277,8 @@ cifs_put_smb_ses(struct cifs_ses *ses)
#ifdef CONFIG_KEYS
-/* strlen("cifs:a:") + INET6_ADDRSTRLEN + 1 */
-#define CIFSCREDS_DESC_SIZE (7 + INET6_ADDRSTRLEN + 1)
+/* strlen("cifs:a:") + MAX_DOMAINNAME_SIZE + 1 */
+#define CIFSCREDS_DESC_SIZE (7 + MAX_DOMAINNAME_SIZE + 1)
/* Populate username and pw fields from keyring if possible */
static int
diff --git a/fs/cifs/sess.c b/fs/cifs/sess.c
index 79358e3..57b1537 100644
--- a/fs/cifs/sess.c
+++ b/fs/cifs/sess.c
@@ -197,7 +197,7 @@ static void unicode_domain_string(char **pbcc_area, struct cifs_ses *ses,
bytes_ret = 0;
} else
bytes_ret = cifs_strtoUTF16((__le16 *) bcc_ptr, ses->domainName,
- 256, nls_cp);
+ MAX_DOMAINNAME_SIZE, nls_cp);
bcc_ptr += 2 * bytes_ret;
bcc_ptr += 2; /* account for null terminator */
@@ -255,8 +255,8 @@ static void ascii_ssetup_strings(char **pbcc_area, struct cifs_ses *ses,
/* copy domain */
if (ses->domainName != NULL) {
- strncpy(bcc_ptr, ses->domainName, 256);
- bcc_ptr += strnlen(ses->domainName, 256);
+ strncpy(bcc_ptr, ses->domainName, MAX_DOMAINNAME_SIZE);
+ bcc_ptr += strnlen(ses->domainName, MAX_DOMAINNAME_SIZE);
} /* else we will send a null domain name
so the server will default to its own domain */
*bcc_ptr = 0;

Looks good...

[Steve French]

Post by Steve French
merged into cifs-2.6.git

Post by Jeff Layton
On Fri, 19 Jul 2013 09:01:36 +0800

Post by Chen Gang
For cifs_set_cifscreds() in "fs/cifs/connect.c", 'desc' buffer length
is 'CIFSCREDS_DESC_SIZE' (56 is less than 256), and 'ses->domainName'
length may be "255 + '\0'".
The related sprintf() may cause memory overflow, so need extend related
buffer enough to hold all things.
It is also necessary to be sure of 'ses->domainName' must be less than
256, and define the related macro instead of hard code number '256'.
---
fs/cifs/cifsencrypt.c | 2 +-
fs/cifs/cifsglob.h | 1 +
fs/cifs/connect.c | 7 ++++---
fs/cifs/sess.c | 6 +++---
4 files changed, 9 insertions(+), 7 deletions(-)
diff --git a/fs/cifs/cifsencrypt.c b/fs/cifs/cifsencrypt.c
index 45e57cc..bb84d7c 100644
--- a/fs/cifs/cifsencrypt.c
+++ b/fs/cifs/cifsencrypt.c
@@ -421,7 +421,7 @@ find_domain_name(struct cifs_ses *ses, const struct nls_table *nls_cp)
if (blobptr + attrsize > blobend)
break;
if (type == NTLMSSP_AV_NB_DOMAIN_NAME) {
- if (!attrsize)
+ if (!attrsize || attrsize >= MAX_DOMAINNAME_SIZE)
break;
if (!ses->domainName) {
ses->domainName =
diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
index 33f17ed..64bb4c5 100644
--- a/fs/cifs/cifsglob.h
+++ b/fs/cifs/cifsglob.h
@@ -44,6 +44,7 @@
#define MAX_TREE_SIZE (2 + MAX_SERVER_SIZE + 1 + MAX_SHARE_SIZE + 1)
#define MAX_SERVER_SIZE 15
#define MAX_SHARE_SIZE 80
+#define MAX_DOMAINNAME_SIZE 256 /* maximum fully qualified domain name (FQDN) */
#define MAX_USERNAME_SIZE 256 /* reasonable maximum for current servers */
#define MAX_PASSWORD_SIZE 512 /* max for windows seems to be 256 wide chars */
diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
index fa68813..3f9dcf7 100644
--- a/fs/cifs/connect.c
+++ b/fs/cifs/connect.c
@@ -1675,7 +1675,8 @@ cifs_parse_mount_options(const char *mountdata, const char *devname,
if (string == NULL)
goto out_nomem;
- if (strnlen(string, 256) == 256) {
+ if (strnlen(string, MAX_DOMAINNAME_SIZE)
+ == MAX_DOMAINNAME_SIZE) {
printk(KERN_WARNING "CIFS: domain name too"
" long\n");
goto cifs_parse_mount_err;
@@ -2276,8 +2277,8 @@ cifs_put_smb_ses(struct cifs_ses *ses)
#ifdef CONFIG_KEYS
-/* strlen("cifs:a:") + INET6_ADDRSTRLEN + 1 */
-#define CIFSCREDS_DESC_SIZE (7 + INET6_ADDRSTRLEN + 1)
+/* strlen("cifs:a:") + MAX_DOMAINNAME_SIZE + 1 */
+#define CIFSCREDS_DESC_SIZE (7 + MAX_DOMAINNAME_SIZE + 1)
/* Populate username and pw fields from keyring if possible */
static int
diff --git a/fs/cifs/sess.c b/fs/cifs/sess.c
index 79358e3..57b1537 100644
--- a/fs/cifs/sess.c
+++ b/fs/cifs/sess.c
@@ -197,7 +197,7 @@ static void unicode_domain_string(char **pbcc_area, struct cifs_ses *ses,
bytes_ret = 0;
} else
bytes_ret = cifs_strtoUTF16((__le16 *) bcc_ptr, ses->domainName,
- 256, nls_cp);
+ MAX_DOMAINNAME_SIZE, nls_cp);
bcc_ptr += 2 * bytes_ret;
bcc_ptr += 2; /* account for null terminator */
@@ -255,8 +255,8 @@ static void ascii_ssetup_strings(char **pbcc_area, struct cifs_ses *ses,
/* copy domain */
if (ses->domainName != NULL) {
- strncpy(bcc_ptr, ses->domainName, 256);
- bcc_ptr += strnlen(ses->domainName, 256);
+ strncpy(bcc_ptr, ses->domainName, MAX_DOMAINNAME_SIZE);
+ bcc_ptr += strnlen(ses->domainName, MAX_DOMAINNAME_SIZE);
} /* else we will send a null domain name
so the server will default to its own domain */
*bcc_ptr = 0;

Looks good...

--
Thanks,
Steve

[Chen Gang]

Post by Steve French
merged into cifs-2.6.git

Thanks.

Post by Steve French

Post by Jeff Layton
On Fri, 19 Jul 2013 09:01:36 +0800

Post by Chen Gang
For cifs_set_cifscreds() in "fs/cifs/connect.c", 'desc' buffer length
is 'CIFSCREDS_DESC_SIZE' (56 is less than 256), and 'ses->domainName'
length may be "255 + '\0'".
The related sprintf() may cause memory overflow, so need extend related
buffer enough to hold all things.
It is also necessary to be sure of 'ses->domainName' must be less than
256, and define the related macro instead of hard code number '256'.
---
fs/cifs/cifsencrypt.c | 2 +-
fs/cifs/cifsglob.h | 1 +
fs/cifs/connect.c | 7 ++++---
fs/cifs/sess.c | 6 +++---
4 files changed, 9 insertions(+), 7 deletions(-)
diff --git a/fs/cifs/cifsencrypt.c b/fs/cifs/cifsencrypt.c
index 45e57cc..bb84d7c 100644
--- a/fs/cifs/cifsencrypt.c
+++ b/fs/cifs/cifsencrypt.c
@@ -421,7 +421,7 @@ find_domain_name(struct cifs_ses *ses, const struct nls_table *nls_cp)
if (blobptr + attrsize > blobend)
break;
if (type == NTLMSSP_AV_NB_DOMAIN_NAME) {
- if (!attrsize)
+ if (!attrsize || attrsize >= MAX_DOMAINNAME_SIZE)
break;
if (!ses->domainName) {
ses->domainName =
diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
index 33f17ed..64bb4c5 100644
--- a/fs/cifs/cifsglob.h
+++ b/fs/cifs/cifsglob.h
@@ -44,6 +44,7 @@
#define MAX_TREE_SIZE (2 + MAX_SERVER_SIZE + 1 + MAX_SHARE_SIZE + 1)
#define MAX_SERVER_SIZE 15
#define MAX_SHARE_SIZE 80
+#define MAX_DOMAINNAME_SIZE 256 /* maximum fully qualified domain name (FQDN) */
#define MAX_USERNAME_SIZE 256 /* reasonable maximum for current servers */
#define MAX_PASSWORD_SIZE 512 /* max for windows seems to be 256 wide chars */
diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
index fa68813..3f9dcf7 100644
--- a/fs/cifs/connect.c
+++ b/fs/cifs/connect.c
@@ -1675,7 +1675,8 @@ cifs_parse_mount_options(const char *mountdata, const char *devname,
if (string == NULL)
goto out_nomem;
- if (strnlen(string, 256) == 256) {
+ if (strnlen(string, MAX_DOMAINNAME_SIZE)
+ == MAX_DOMAINNAME_SIZE) {
printk(KERN_WARNING "CIFS: domain name too"
" long\n");
goto cifs_parse_mount_err;
@@ -2276,8 +2277,8 @@ cifs_put_smb_ses(struct cifs_ses *ses)
#ifdef CONFIG_KEYS
-/* strlen("cifs:a:") + INET6_ADDRSTRLEN + 1 */
-#define CIFSCREDS_DESC_SIZE (7 + INET6_ADDRSTRLEN + 1)
+/* strlen("cifs:a:") + MAX_DOMAINNAME_SIZE + 1 */
+#define CIFSCREDS_DESC_SIZE (7 + MAX_DOMAINNAME_SIZE + 1)
/* Populate username and pw fields from keyring if possible */
static int
diff --git a/fs/cifs/sess.c b/fs/cifs/sess.c
index 79358e3..57b1537 100644
--- a/fs/cifs/sess.c
+++ b/fs/cifs/sess.c
@@ -197,7 +197,7 @@ static void unicode_domain_string(char **pbcc_area, struct cifs_ses *ses,
bytes_ret = 0;
} else
bytes_ret = cifs_strtoUTF16((__le16 *) bcc_ptr, ses->domainName,
- 256, nls_cp);
+ MAX_DOMAINNAME_SIZE, nls_cp);
bcc_ptr += 2 * bytes_ret;
bcc_ptr += 2; /* account for null terminator */
@@ -255,8 +255,8 @@ static void ascii_ssetup_strings(char **pbcc_area, struct cifs_ses *ses,
/* copy domain */
if (ses->domainName != NULL) {
- strncpy(bcc_ptr, ses->domainName, 256);
- bcc_ptr += strnlen(ses->domainName, 256);
+ strncpy(bcc_ptr, ses->domainName, MAX_DOMAINNAME_SIZE);
+ bcc_ptr += strnlen(ses->domainName, MAX_DOMAINNAME_SIZE);
} /* else we will send a null domain name
so the server will default to its own domain */
*bcc_ptr = 0;

Looks good...

-- Thanks, Steve

[Steve French]

I updated the version of the patch slightly which is in cifs-2.6.git
so we don't have to rename the MAX_DOMAINNAME_SIZE field multiple
times. Let me know if you see problems. It could make Scott's
followup patch a little easier to understand without the renamed
#define.

commit 057d6332b24a4497c55a761c83c823eed9e3f23b
Author: Chen Gang <gang.chen-bOixZGp5f+***@public.gmane.org>
Date: Fri Jul 19 09:01:36 2013 +0800

cifs: extend the buffer length enought for sprintf() using

For cifs_set_cifscreds() in "fs/cifs/connect.c", 'desc' buffer length
is 'CIFSCREDS_DESC_SIZE' (56 is less than 256), and 'ses->domainName'
length may be "255 + '\0'".

The related sprintf() may cause memory overflow, so need extend related
buffer enough to hold all things.

It is also necessary to be sure of 'ses->domainName' must be less than
256, and define the related macro instead of hard code number '256'.

Signed-off-by: Chen Gang <gang.chen-bOixZGp5f+***@public.gmane.org>
Reviewed-by: Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+***@public.gmane.org>
Reviewed-by: Shirish Pargaonkar <shirishpargaonkar-***@public.gmane.org>
Reviewed-by: Scott Lovenberg <scott.lovenberg-***@public.gmane.org>
CC: <stable-***@public.gmane.org>
Signed-off-by: Steve French <smfrench-***@public.gmane.org>

diff --git a/fs/cifs/cifsencrypt.c b/fs/cifs/cifsencrypt.c
index 45e57cc..194f9cc 100644
--- a/fs/cifs/cifsencrypt.c
+++ b/fs/cifs/cifsencrypt.c
@@ -421,7 +421,7 @@ find_domain_name(struct cifs_ses *ses, const
struct nls_table *nls_cp)
if (blobptr + attrsize > blobend)
break;
if (type == NTLMSSP_AV_NB_DOMAIN_NAME) {
- if (!attrsize)
+ if (!attrsize || attrsize >= CIFS_MAX_DOMAINNAME_LEN)
break;
if (!ses->domainName) {
ses->domainName =
diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
index 1fdc370..0e68893 100644
--- a/fs/cifs/cifsglob.h
+++ b/fs/cifs/cifsglob.h
@@ -44,6 +44,7 @@
#define MAX_TREE_SIZE (2 + MAX_SERVER_SIZE + 1 + MAX_SHARE_SIZE + 1)
#define MAX_SERVER_SIZE 15
#define MAX_SHARE_SIZE 80
+#define CIFS_MAX_DOMAINNAME_LEN 256 /* max domain name length */
#define MAX_USERNAME_SIZE 256 /* reasonable maximum for current servers */
#define MAX_PASSWORD_SIZE 512 /* max for windows seems to be 256 wide chars */

diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
index fa68813..d67c550 100644
--- a/fs/cifs/connect.c
+++ b/fs/cifs/connect.c
@@ -1675,7 +1675,8 @@ cifs_parse_mount_options(const char *mountdata,
const char *devname,
if (string == NULL)
goto out_nomem;

- if (strnlen(string, 256) == 256) {
+ if (strnlen(string, CIFS_MAX_DOMAINNAME_LEN)
+ == CIFS_MAX_DOMAINNAME_LEN) {
printk(KERN_WARNING "CIFS: domain name too"
" long\n");
goto cifs_parse_mount_err;
@@ -2276,8 +2277,8 @@ cifs_put_smb_ses(struct cifs_ses *ses)

#ifdef CONFIG_KEYS

-/* strlen("cifs:a:") + INET6_ADDRSTRLEN + 1 */
-#define CIFSCREDS_DESC_SIZE (7 + INET6_ADDRSTRLEN + 1)
+/* strlen("cifs:a:") + CIFS_MAX_DOMAINNAME_LEN + 1 */
+#define CIFSCREDS_DESC_SIZE (7 + CIFS_MAX_DOMAINNAME_LEN + 1)

/* Populate username and pw fields from keyring if possible */
static int
diff --git a/fs/cifs/sess.c b/fs/cifs/sess.c
index 79358e3..08dd37b 100644
--- a/fs/cifs/sess.c
+++ b/fs/cifs/sess.c
@@ -197,7 +197,7 @@ static void unicode_domain_string(char
**pbcc_area, struct cifs_ses *ses,
bytes_ret = 0;
} else
bytes_ret = cifs_strtoUTF16((__le16 *) bcc_ptr, ses->domainName,
- 256, nls_cp);
+ CIFS_MAX_DOMAINNAME_LEN, nls_cp);
bcc_ptr += 2 * bytes_ret;
bcc_ptr += 2; /* account for null terminator */

@@ -255,8 +255,8 @@ static void ascii_ssetup_strings(char **pbcc_area,
struct cifs_ses *ses,

/* copy domain */
if (ses->domainName != NULL) {
- strncpy(bcc_ptr, ses->domainName, 256);
- bcc_ptr += strnlen(ses->domainName, 256);
+ strncpy(bcc_ptr, ses->domainName, CIFS_MAX_DOMAINNAME_LEN);
+ bcc_ptr += strnlen(ses->domainName, CIFS_MAX_DOMAINNAME_LEN);
} /* else we will send a null domain name
so the server will default to its own domain */
*bcc_ptr = 0;

Post by Chen Gang

Post by Steve French
merged into cifs-2.6.git

Thanks.

Post by Steve French

Post by Jeff Layton
On Fri, 19 Jul 2013 09:01:36 +0800

Post by Chen Gang
For cifs_set_cifscreds() in "fs/cifs/connect.c", 'desc' buffer length
is 'CIFSCREDS_DESC_SIZE' (56 is less than 256), and 'ses->domainName'
length may be "255 + '\0'".
The related sprintf() may cause memory overflow, so need extend related
buffer enough to hold all things.
It is also necessary to be sure of 'ses->domainName' must be less than
256, and define the related macro instead of hard code number '256'.
---
fs/cifs/cifsencrypt.c | 2 +-
fs/cifs/cifsglob.h | 1 +
fs/cifs/connect.c | 7 ++++---
fs/cifs/sess.c | 6 +++---
4 files changed, 9 insertions(+), 7 deletions(-)
diff --git a/fs/cifs/cifsencrypt.c b/fs/cifs/cifsencrypt.c
index 45e57cc..bb84d7c 100644
--- a/fs/cifs/cifsencrypt.c
+++ b/fs/cifs/cifsencrypt.c
@@ -421,7 +421,7 @@ find_domain_name(struct cifs_ses *ses, const struct nls_table *nls_cp)
if (blobptr + attrsize > blobend)
break;
if (type == NTLMSSP_AV_NB_DOMAIN_NAME) {
- if (!attrsize)
+ if (!attrsize || attrsize >= MAX_DOMAINNAME_SIZE)
break;
if (!ses->domainName) {
ses->domainName =
diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
index 33f17ed..64bb4c5 100644
--- a/fs/cifs/cifsglob.h
+++ b/fs/cifs/cifsglob.h
@@ -44,6 +44,7 @@
#define MAX_TREE_SIZE (2 + MAX_SERVER_SIZE + 1 + MAX_SHARE_SIZE + 1)
#define MAX_SERVER_SIZE 15
#define MAX_SHARE_SIZE 80
+#define MAX_DOMAINNAME_SIZE 256 /* maximum fully qualified domain name (FQDN) */
#define MAX_USERNAME_SIZE 256 /* reasonable maximum for current servers */
#define MAX_PASSWORD_SIZE 512 /* max for windows seems to be 256 wide chars */
diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
index fa68813..3f9dcf7 100644
--- a/fs/cifs/connect.c
+++ b/fs/cifs/connect.c
@@ -1675,7 +1675,8 @@ cifs_parse_mount_options(const char *mountdata, const char *devname,
if (string == NULL)
goto out_nomem;
- if (strnlen(string, 256) == 256) {
+ if (strnlen(string, MAX_DOMAINNAME_SIZE)
+ == MAX_DOMAINNAME_SIZE) {
printk(KERN_WARNING "CIFS: domain name too"
" long\n");
goto cifs_parse_mount_err;
@@ -2276,8 +2277,8 @@ cifs_put_smb_ses(struct cifs_ses *ses)
#ifdef CONFIG_KEYS
-/* strlen("cifs:a:") + INET6_ADDRSTRLEN + 1 */
-#define CIFSCREDS_DESC_SIZE (7 + INET6_ADDRSTRLEN + 1)
+/* strlen("cifs:a:") + MAX_DOMAINNAME_SIZE + 1 */
+#define CIFSCREDS_DESC_SIZE (7 + MAX_DOMAINNAME_SIZE + 1)
/* Populate username and pw fields from keyring if possible */
static int
diff --git a/fs/cifs/sess.c b/fs/cifs/sess.c
index 79358e3..57b1537 100644
--- a/fs/cifs/sess.c
+++ b/fs/cifs/sess.c
@@ -197,7 +197,7 @@ static void unicode_domain_string(char **pbcc_area, struct cifs_ses *ses,
bytes_ret = 0;
} else
bytes_ret = cifs_strtoUTF16((__le16 *) bcc_ptr, ses->domainName,
- 256, nls_cp);
+ MAX_DOMAINNAME_SIZE, nls_cp);
bcc_ptr += 2 * bytes_ret;
bcc_ptr += 2; /* account for null terminator */
@@ -255,8 +255,8 @@ static void ascii_ssetup_strings(char **pbcc_area, struct cifs_ses *ses,
/* copy domain */
if (ses->domainName != NULL) {
- strncpy(bcc_ptr, ses->domainName, 256);
- bcc_ptr += strnlen(ses->domainName, 256);
+ strncpy(bcc_ptr, ses->domainName, MAX_DOMAINNAME_SIZE);
+ bcc_ptr += strnlen(ses->domainName, MAX_DOMAINNAME_SIZE);
} /* else we will send a null domain name
so the server will default to its own domain */
*bcc_ptr = 0;

Looks good...

-- Thanks, Steve

--
Chen Gang

[Scott Lovenberg]

Post by Steve French
I updated the version of the patch slightly which is in cifs-2.6.git
so we don't have to rename the MAX_DOMAINNAME_SIZE field multiple
times. Let me know if you see problems. It could make Scott's
followup patch a little easier to understand without the renamed
#define.
--
Thanks,
Steve

+1. I agree it streamlines the other patch set.

[Chen Gang]

Post by Jeff Layton
On Fri, 19 Jul 2013 09:01:36 +0800

Post by Chen Gang
For cifs_set_cifscreds() in "fs/cifs/connect.c", 'desc' buffer length
is 'CIFSCREDS_DESC_SIZE' (56 is less than 256), and 'ses->domainName'
length may be "255 + '\0'".
The related sprintf() may cause memory overflow, so need extend related
buffer enough to hold all things.
It is also necessary to be sure of 'ses->domainName' must be less than
256, and define the related macro instead of hard code number '256'.
---
fs/cifs/cifsencrypt.c | 2 +-
fs/cifs/cifsglob.h | 1 +
fs/cifs/connect.c | 7 ++++---
fs/cifs/sess.c | 6 +++---
4 files changed, 9 insertions(+), 7 deletions(-)
diff --git a/fs/cifs/cifsencrypt.c b/fs/cifs/cifsencrypt.c
index 45e57cc..bb84d7c 100644
--- a/fs/cifs/cifsencrypt.c
+++ b/fs/cifs/cifsencrypt.c
@@ -421,7 +421,7 @@ find_domain_name(struct cifs_ses *ses, const struct nls_table *nls_cp)
if (blobptr + attrsize > blobend)
break;
if (type == NTLMSSP_AV_NB_DOMAIN_NAME) {
- if (!attrsize)
+ if (!attrsize || attrsize >= MAX_DOMAINNAME_SIZE)
break;
if (!ses->domainName) {
ses->domainName =
diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
index 33f17ed..64bb4c5 100644
--- a/fs/cifs/cifsglob.h
+++ b/fs/cifs/cifsglob.h
@@ -44,6 +44,7 @@
#define MAX_TREE_SIZE (2 + MAX_SERVER_SIZE + 1 + MAX_SHARE_SIZE + 1)
#define MAX_SERVER_SIZE 15
#define MAX_SHARE_SIZE 80
+#define MAX_DOMAINNAME_SIZE 256 /* maximum fully qualified domain name (FQDN) */
#define MAX_USERNAME_SIZE 256 /* reasonable maximum for current servers */
#define MAX_PASSWORD_SIZE 512 /* max for windows seems to be 256 wide chars */
diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
index fa68813..3f9dcf7 100644
--- a/fs/cifs/connect.c
+++ b/fs/cifs/connect.c
@@ -1675,7 +1675,8 @@ cifs_parse_mount_options(const char *mountdata, const char *devname,
if (string == NULL)
goto out_nomem;
- if (strnlen(string, 256) == 256) {
+ if (strnlen(string, MAX_DOMAINNAME_SIZE)
+ == MAX_DOMAINNAME_SIZE) {
printk(KERN_WARNING "CIFS: domain name too"
" long\n");
goto cifs_parse_mount_err;
@@ -2276,8 +2277,8 @@ cifs_put_smb_ses(struct cifs_ses *ses)
#ifdef CONFIG_KEYS
-/* strlen("cifs:a:") + INET6_ADDRSTRLEN + 1 */
-#define CIFSCREDS_DESC_SIZE (7 + INET6_ADDRSTRLEN + 1)
+/* strlen("cifs:a:") + MAX_DOMAINNAME_SIZE + 1 */
+#define CIFSCREDS_DESC_SIZE (7 + MAX_DOMAINNAME_SIZE + 1)
/* Populate username and pw fields from keyring if possible */
static int
diff --git a/fs/cifs/sess.c b/fs/cifs/sess.c
index 79358e3..57b1537 100644
--- a/fs/cifs/sess.c
+++ b/fs/cifs/sess.c
@@ -197,7 +197,7 @@ static void unicode_domain_string(char **pbcc_area, struct cifs_ses *ses,
bytes_ret = 0;
} else
bytes_ret = cifs_strtoUTF16((__le16 *) bcc_ptr, ses->domainName,
- 256, nls_cp);
+ MAX_DOMAINNAME_SIZE, nls_cp);
bcc_ptr += 2 * bytes_ret;
bcc_ptr += 2; /* account for null terminator */
@@ -255,8 +255,8 @@ static void ascii_ssetup_strings(char **pbcc_area, struct cifs_ses *ses,
/* copy domain */
if (ses->domainName != NULL) {
- strncpy(bcc_ptr, ses->domainName, 256);
- bcc_ptr += strnlen(ses->domainName, 256);
+ strncpy(bcc_ptr, ses->domainName, MAX_DOMAINNAME_SIZE);
+ bcc_ptr += strnlen(ses->domainName, MAX_DOMAINNAME_SIZE);
} /* else we will send a null domain name
so the server will default to its own domain */
*bcc_ptr = 0;

Looks good...

Thanks.

[Chen Gang]

Post by Scott Lovenberg

Post by Chen Gang
For cifs_set_cifscreds() in "fs/cifs/connect.c", 'desc' buffer length
is 'CIFSCREDS_DESC_SIZE' (56 is less than 256), and 'ses->domainName'
length may be "255 + '\0'".
The related sprintf() may cause memory overflow, so need extend related
buffer enough to hold all things.
It is also necessary to be sure of 'ses->domainName' must be less than
256, and define the related macro instead of hard code number '256'.
---
fs/cifs/cifsencrypt.c | 2 +-
fs/cifs/cifsglob.h | 1 +
fs/cifs/connect.c | 7 ++++---
fs/cifs/sess.c | 6 +++---
4 files changed, 9 insertions(+), 7 deletions(-)
diff --git a/fs/cifs/cifsencrypt.c b/fs/cifs/cifsencrypt.c
index 45e57cc..ce2d331 100644
--- a/fs/cifs/cifsencrypt.c
+++ b/fs/cifs/cifsencrypt.c
@@ -421,7 +421,7 @@ find_domain_name(struct cifs_ses *ses, const struct nls_table *nls_cp)
if (blobptr + attrsize > blobend)
break;
if (type == NTLMSSP_AV_NB_DOMAIN_NAME) {
- if (!attrsize)
+ if (!attrsize || attrsize >= MAX_CIF_DOMAINNAME)
break;
if (!ses->domainName) {
ses->domainName =
diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
index 33f17ed..88280e0 100644
--- a/fs/cifs/cifsglob.h
+++ b/fs/cifs/cifsglob.h
@@ -396,6 +396,7 @@ struct smb_version_values {
#define HEADER_SIZE(server) (server->vals->header_size)
#define MAX_HEADER_SIZE(server) (server->vals->max_header_size)
+#define MAX_CIF_DOMAINNAME 256 /* 255 + '\0' */
struct smb_vol {
char *username;
diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
index fa68813..ed6bf20 100644
--- a/fs/cifs/connect.c
+++ b/fs/cifs/connect.c
@@ -1675,7 +1675,8 @@ cifs_parse_mount_options(const char *mountdata, const char *devname,
if (string == NULL)
goto out_nomem;
- if (strnlen(string, 256) == 256) {
+ if (strnlen(string, MAX_CIF_DOMAINNAME)
+ == MAX_CIF_DOMAINNAME) {
printk(KERN_WARNING "CIFS: domain name too"
" long\n");
goto cifs_parse_mount_err;
@@ -2276,8 +2277,8 @@ cifs_put_smb_ses(struct cifs_ses *ses)
#ifdef CONFIG_KEYS
-/* strlen("cifs:a:") + INET6_ADDRSTRLEN + 1 */
-#define CIFSCREDS_DESC_SIZE (7 + INET6_ADDRSTRLEN + 1)
+/* strlen("cifs:a:") + MAX_CIF_DOMAINNAME + 1 */
+#define CIFSCREDS_DESC_SIZE (7 + MAX_CIF_DOMAINNAME + 1)
/* Populate username and pw fields from keyring if possible */
static int
diff --git a/fs/cifs/sess.c b/fs/cifs/sess.c
index 79358e3..106a575 100644
--- a/fs/cifs/sess.c
+++ b/fs/cifs/sess.c
@@ -197,7 +197,7 @@ static void unicode_domain_string(char **pbcc_area, struct cifs_ses *ses,
bytes_ret = 0;
} else
bytes_ret = cifs_strtoUTF16((__le16 *) bcc_ptr, ses->domainName,
- 256, nls_cp);
+ MAX_CIF_DOMAINNAME, nls_cp);
bcc_ptr += 2 * bytes_ret;
bcc_ptr += 2; /* account for null terminator */
@@ -255,8 +255,8 @@ static void ascii_ssetup_strings(char **pbcc_area, struct cifs_ses *ses,
/* copy domain */
if (ses->domainName != NULL) {
- strncpy(bcc_ptr, ses->domainName, 256);
- bcc_ptr += strnlen(ses->domainName, 256);
+ strncpy(bcc_ptr, ses->domainName, MAX_CIF_DOMAINNAME);
+ bcc_ptr += strnlen(ses->domainName, MAX_CIF_DOMAINNAME);
} /* else we will send a null domain name
so the server will default to its own domain */
*bcc_ptr = 0;
--
1.7.7.6
--
To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
More majordomo info at http://vger.kernel.org/majordomo-info.html

If this is correct for the domain size, MAX_DOMAIN_SIZE in the
cifs-utils mount helper (mount.cifs.c) has to be changed. It is
currently 64 + null terminator. I can open a bug and patch it if this
is correct.
CC'ing Jeff Layton since he maintains the cifs-utils package.

Thank you very much.

[Jeff Layton]

On Tue, 16 Jul 2013 21:58:19 -0400

Post by Scott Lovenberg

Post by Chen Gang
For cifs_set_cifscreds() in "fs/cifs/connect.c", 'desc' buffer length
is 'CIFSCREDS_DESC_SIZE' (56 is less than 256), and 'ses->domainName'
length may be "255 + '\0'".
The related sprintf() may cause memory overflow, so need extend related
buffer enough to hold all things.
It is also necessary to be sure of 'ses->domainName' must be less than
256, and define the related macro instead of hard code number '256'.
---
fs/cifs/cifsencrypt.c | 2 +-
fs/cifs/cifsglob.h | 1 +
fs/cifs/connect.c | 7 ++++---
fs/cifs/sess.c | 6 +++---
4 files changed, 9 insertions(+), 7 deletions(-)
diff --git a/fs/cifs/cifsencrypt.c b/fs/cifs/cifsencrypt.c
index 45e57cc..ce2d331 100644
--- a/fs/cifs/cifsencrypt.c
+++ b/fs/cifs/cifsencrypt.c
@@ -421,7 +421,7 @@ find_domain_name(struct cifs_ses *ses, const struct nls_table *nls_cp)
if (blobptr + attrsize > blobend)
break;
if (type == NTLMSSP_AV_NB_DOMAIN_NAME) {
- if (!attrsize)
+ if (!attrsize || attrsize >= MAX_CIF_DOMAINNAME)
break;
if (!ses->domainName) {
ses->domainName =
diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
index 33f17ed..88280e0 100644
--- a/fs/cifs/cifsglob.h
+++ b/fs/cifs/cifsglob.h
@@ -396,6 +396,7 @@ struct smb_version_values {
#define HEADER_SIZE(server) (server->vals->header_size)
#define MAX_HEADER_SIZE(server) (server->vals->max_header_size)
+#define MAX_CIF_DOMAINNAME 256 /* 255 + '\0' */
struct smb_vol {
char *username;
diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
index fa68813..ed6bf20 100644
--- a/fs/cifs/connect.c
+++ b/fs/cifs/connect.c
@@ -1675,7 +1675,8 @@ cifs_parse_mount_options(const char *mountdata, const char *devname,
if (string == NULL)
goto out_nomem;
- if (strnlen(string, 256) == 256) {
+ if (strnlen(string, MAX_CIF_DOMAINNAME)
+ == MAX_CIF_DOMAINNAME) {
printk(KERN_WARNING "CIFS: domain name too"
" long\n");
goto cifs_parse_mount_err;
@@ -2276,8 +2277,8 @@ cifs_put_smb_ses(struct cifs_ses *ses)
#ifdef CONFIG_KEYS
-/* strlen("cifs:a:") + INET6_ADDRSTRLEN + 1 */
-#define CIFSCREDS_DESC_SIZE (7 + INET6_ADDRSTRLEN + 1)
+/* strlen("cifs:a:") + MAX_CIF_DOMAINNAME + 1 */
+#define CIFSCREDS_DESC_SIZE (7 + MAX_CIF_DOMAINNAME + 1)
/* Populate username and pw fields from keyring if possible */
static int
diff --git a/fs/cifs/sess.c b/fs/cifs/sess.c
index 79358e3..106a575 100644
--- a/fs/cifs/sess.c
+++ b/fs/cifs/sess.c
@@ -197,7 +197,7 @@ static void unicode_domain_string(char **pbcc_area, struct cifs_ses *ses,
bytes_ret = 0;
} else
bytes_ret = cifs_strtoUTF16((__le16 *) bcc_ptr, ses->domainName,
- 256, nls_cp);
+ MAX_CIF_DOMAINNAME, nls_cp);
bcc_ptr += 2 * bytes_ret;
bcc_ptr += 2; /* account for null terminator */
@@ -255,8 +255,8 @@ static void ascii_ssetup_strings(char **pbcc_area, struct cifs_ses *ses,
/* copy domain */
if (ses->domainName != NULL) {
- strncpy(bcc_ptr, ses->domainName, 256);
- bcc_ptr += strnlen(ses->domainName, 256);
+ strncpy(bcc_ptr, ses->domainName, MAX_CIF_DOMAINNAME);
+ bcc_ptr += strnlen(ses->domainName, MAX_CIF_DOMAINNAME);
} /* else we will send a null domain name
so the server will default to its own domain */
*bcc_ptr = 0;
--
1.7.7.6
--
To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
More majordomo info at http://vger.kernel.org/majordomo-info.html

If this is correct for the domain size, MAX_DOMAIN_SIZE in the
cifs-utils mount helper (mount.cifs.c) has to be changed. It is
currently 64 + null terminator. I can open a bug and patch it if this
is correct.
CC'ing Jeff Layton since he maintains the cifs-utils package.

Yep, it probably should be extended out to 256. Send a patch when you
get a chance and I'll plan to review and merge it.

[Scott Lovenberg]

Post by Jeff Layton
On Tue, 16 Jul 2013 21:58:19 -0400

Post by Scott Lovenberg

Post by Chen Gang
For cifs_set_cifscreds() in "fs/cifs/connect.c", 'desc' buffer length
is 'CIFSCREDS_DESC_SIZE' (56 is less than 256), and 'ses->domainName'
length may be "255 + '\0'".
The related sprintf() may cause memory overflow, so need extend related
buffer enough to hold all things.
It is also necessary to be sure of 'ses->domainName' must be less than
256, and define the related macro instead of hard code number '256'.
---
fs/cifs/cifsencrypt.c | 2 +-
fs/cifs/cifsglob.h | 1 +
fs/cifs/connect.c | 7 ++++---
fs/cifs/sess.c | 6 +++---
4 files changed, 9 insertions(+), 7 deletions(-)
diff --git a/fs/cifs/cifsencrypt.c b/fs/cifs/cifsencrypt.c
index 45e57cc..ce2d331 100644
--- a/fs/cifs/cifsencrypt.c
+++ b/fs/cifs/cifsencrypt.c
@@ -421,7 +421,7 @@ find_domain_name(struct cifs_ses *ses, const struct nls_table *nls_cp)
if (blobptr + attrsize > blobend)
break;
if (type == NTLMSSP_AV_NB_DOMAIN_NAME) {
- if (!attrsize)
+ if (!attrsize || attrsize >= MAX_CIF_DOMAINNAME)
break;
if (!ses->domainName) {
ses->domainName =
diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
index 33f17ed..88280e0 100644
--- a/fs/cifs/cifsglob.h
+++ b/fs/cifs/cifsglob.h
@@ -396,6 +396,7 @@ struct smb_version_values {
#define HEADER_SIZE(server) (server->vals->header_size)
#define MAX_HEADER_SIZE(server) (server->vals->max_header_size)
+#define MAX_CIF_DOMAINNAME 256 /* 255 + '\0' */
struct smb_vol {
char *username;
diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
index fa68813..ed6bf20 100644
--- a/fs/cifs/connect.c
+++ b/fs/cifs/connect.c
@@ -1675,7 +1675,8 @@ cifs_parse_mount_options(const char *mountdata, const char *devname,
if (string == NULL)
goto out_nomem;
- if (strnlen(string, 256) == 256) {
+ if (strnlen(string, MAX_CIF_DOMAINNAME)
+ == MAX_CIF_DOMAINNAME) {
printk(KERN_WARNING "CIFS: domain name too"
" long\n");
goto cifs_parse_mount_err;
@@ -2276,8 +2277,8 @@ cifs_put_smb_ses(struct cifs_ses *ses)
#ifdef CONFIG_KEYS
-/* strlen("cifs:a:") + INET6_ADDRSTRLEN + 1 */
-#define CIFSCREDS_DESC_SIZE (7 + INET6_ADDRSTRLEN + 1)
+/* strlen("cifs:a:") + MAX_CIF_DOMAINNAME + 1 */
+#define CIFSCREDS_DESC_SIZE (7 + MAX_CIF_DOMAINNAME + 1)
/* Populate username and pw fields from keyring if possible */
static int
diff --git a/fs/cifs/sess.c b/fs/cifs/sess.c
index 79358e3..106a575 100644
--- a/fs/cifs/sess.c
+++ b/fs/cifs/sess.c
@@ -197,7 +197,7 @@ static void unicode_domain_string(char **pbcc_area, struct cifs_ses *ses,
bytes_ret = 0;
} else
bytes_ret = cifs_strtoUTF16((__le16 *) bcc_ptr, ses->domainName,
- 256, nls_cp);
+ MAX_CIF_DOMAINNAME, nls_cp);
bcc_ptr += 2 * bytes_ret;
bcc_ptr += 2; /* account for null terminator */
@@ -255,8 +255,8 @@ static void ascii_ssetup_strings(char **pbcc_area, struct cifs_ses *ses,
/* copy domain */
if (ses->domainName != NULL) {
- strncpy(bcc_ptr, ses->domainName, 256);
- bcc_ptr += strnlen(ses->domainName, 256);
+ strncpy(bcc_ptr, ses->domainName, MAX_CIF_DOMAINNAME);
+ bcc_ptr += strnlen(ses->domainName, MAX_CIF_DOMAINNAME);
} /* else we will send a null domain name
so the server will default to its own domain */
*bcc_ptr = 0;
--
1.7.7.6
--
To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
More majordomo info at http://vger.kernel.org/majordomo-info.html

If this is correct for the domain size, MAX_DOMAIN_SIZE in the
cifs-utils mount helper (mount.cifs.c) has to be changed. It is
currently 64 + null terminator. I can open a bug and patch it if this
is correct.
CC'ing Jeff Layton since he maintains the cifs-utils package.

Yep, it probably should be extended out to 256. Send a patch when you
get a chance and I'll plan to review and merge it.
--

I can get to it this afternoon. :)

What do you want to do about the conflicting MAX_SHARE_SIZE? The
kernel allows 80, we use 256 in the mount helper. Right now we can
potentially pass in a string that's too long and it'll be kicked back
from the kernel. I guess it could be bad if anyone ever forgets to
check the string before using it on the kernel side. A quick Googling
suggests that the kernel side code is correct
(http://support.microsoft.com/kb/916525) about 80 characters.

I've made the other MAX_* defines match up with the kernel side
because the mount helper is too conservative with string lengths, this
is the only case where it's too liberal.
--
Peace and Blessings,
-Scott.

[Jeff Layton]

On Fri, 19 Jul 2013 15:32:29 -0400

Post by Scott Lovenberg

Post by Jeff Layton
On Tue, 16 Jul 2013 21:58:19 -0400

Post by Scott Lovenberg

Post by Chen Gang
For cifs_set_cifscreds() in "fs/cifs/connect.c", 'desc' buffer length
is 'CIFSCREDS_DESC_SIZE' (56 is less than 256), and 'ses->domainName'
length may be "255 + '\0'".
The related sprintf() may cause memory overflow, so need extend related
buffer enough to hold all things.
It is also necessary to be sure of 'ses->domainName' must be less than
256, and define the related macro instead of hard code number '256'.
---
fs/cifs/cifsencrypt.c | 2 +-
fs/cifs/cifsglob.h | 1 +
fs/cifs/connect.c | 7 ++++---
fs/cifs/sess.c | 6 +++---
4 files changed, 9 insertions(+), 7 deletions(-)
diff --git a/fs/cifs/cifsencrypt.c b/fs/cifs/cifsencrypt.c
index 45e57cc..ce2d331 100644
--- a/fs/cifs/cifsencrypt.c
+++ b/fs/cifs/cifsencrypt.c
@@ -421,7 +421,7 @@ find_domain_name(struct cifs_ses *ses, const struct nls_table *nls_cp)
if (blobptr + attrsize > blobend)
break;
if (type == NTLMSSP_AV_NB_DOMAIN_NAME) {
- if (!attrsize)
+ if (!attrsize || attrsize >= MAX_CIF_DOMAINNAME)
break;
if (!ses->domainName) {
ses->domainName =
diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
index 33f17ed..88280e0 100644
--- a/fs/cifs/cifsglob.h
+++ b/fs/cifs/cifsglob.h
@@ -396,6 +396,7 @@ struct smb_version_values {
#define HEADER_SIZE(server) (server->vals->header_size)
#define MAX_HEADER_SIZE(server) (server->vals->max_header_size)
+#define MAX_CIF_DOMAINNAME 256 /* 255 + '\0' */
struct smb_vol {
char *username;
diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
index fa68813..ed6bf20 100644
--- a/fs/cifs/connect.c
+++ b/fs/cifs/connect.c
@@ -1675,7 +1675,8 @@ cifs_parse_mount_options(const char *mountdata, const char *devname,
if (string == NULL)
goto out_nomem;
- if (strnlen(string, 256) == 256) {
+ if (strnlen(string, MAX_CIF_DOMAINNAME)
+ == MAX_CIF_DOMAINNAME) {
printk(KERN_WARNING "CIFS: domain name too"
" long\n");
goto cifs_parse_mount_err;
@@ -2276,8 +2277,8 @@ cifs_put_smb_ses(struct cifs_ses *ses)
#ifdef CONFIG_KEYS
-/* strlen("cifs:a:") + INET6_ADDRSTRLEN + 1 */
-#define CIFSCREDS_DESC_SIZE (7 + INET6_ADDRSTRLEN + 1)
+/* strlen("cifs:a:") + MAX_CIF_DOMAINNAME + 1 */
+#define CIFSCREDS_DESC_SIZE (7 + MAX_CIF_DOMAINNAME + 1)
/* Populate username and pw fields from keyring if possible */
static int
diff --git a/fs/cifs/sess.c b/fs/cifs/sess.c
index 79358e3..106a575 100644
--- a/fs/cifs/sess.c
+++ b/fs/cifs/sess.c
@@ -197,7 +197,7 @@ static void unicode_domain_string(char **pbcc_area, struct cifs_ses *ses,
bytes_ret = 0;
} else
bytes_ret = cifs_strtoUTF16((__le16 *) bcc_ptr, ses->domainName,
- 256, nls_cp);
+ MAX_CIF_DOMAINNAME, nls_cp);
bcc_ptr += 2 * bytes_ret;
bcc_ptr += 2; /* account for null terminator */
@@ -255,8 +255,8 @@ static void ascii_ssetup_strings(char **pbcc_area, struct cifs_ses *ses,
/* copy domain */
if (ses->domainName != NULL) {
- strncpy(bcc_ptr, ses->domainName, 256);
- bcc_ptr += strnlen(ses->domainName, 256);
+ strncpy(bcc_ptr, ses->domainName, MAX_CIF_DOMAINNAME);
+ bcc_ptr += strnlen(ses->domainName, MAX_CIF_DOMAINNAME);
} /* else we will send a null domain name
so the server will default to its own domain */
*bcc_ptr = 0;
--
1.7.7.6
--
To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
More majordomo info at http://vger.kernel.org/majordomo-info.html

If this is correct for the domain size, MAX_DOMAIN_SIZE in the
cifs-utils mount helper (mount.cifs.c) has to be changed. It is
currently 64 + null terminator. I can open a bug and patch it if this
is correct.
CC'ing Jeff Layton since he maintains the cifs-utils package.

Yep, it probably should be extended out to 256. Send a patch when you
get a chance and I'll plan to review and merge it.
--

I can get to it this afternoon. :)
What do you want to do about the conflicting MAX_SHARE_SIZE? The
kernel allows 80, we use 256 in the mount helper. Right now we can
potentially pass in a string that's too long and it'll be kicked back
from the kernel. I guess it could be bad if anyone ever forgets to
check the string before using it on the kernel side. A quick Googling
suggests that the kernel side code is correct
(http://support.microsoft.com/kb/916525) about 80 characters.
I've made the other MAX_* defines match up with the kernel side
because the mount helper is too conservative with string lengths, this
is the only case where it's too liberal.

Sure, sounds fine. Might as well fix up all that you can find...

[Shirish Pargaonkar]

nitpicking...

Should it be MAX_CIFS_DOMAINNAME instead of MAX_CIF_DOMAINNAME,
unless CIF is short for something here?

Regards,

Shirish

Post by Chen Gang
For cifs_set_cifscreds() in "fs/cifs/connect.c", 'desc' buffer length
is 'CIFSCREDS_DESC_SIZE' (56 is less than 256), and 'ses->domainName'
length may be "255 + '\0'".
The related sprintf() may cause memory overflow, so need extend related
buffer enough to hold all things.
It is also necessary to be sure of 'ses->domainName' must be less than
256, and define the related macro instead of hard code number '256'.
---
fs/cifs/cifsencrypt.c | 2 +-
fs/cifs/cifsglob.h | 1 +
fs/cifs/connect.c | 7 ++++---
fs/cifs/sess.c | 6 +++---
4 files changed, 9 insertions(+), 7 deletions(-)
diff --git a/fs/cifs/cifsencrypt.c b/fs/cifs/cifsencrypt.c
index 45e57cc..ce2d331 100644
--- a/fs/cifs/cifsencrypt.c
+++ b/fs/cifs/cifsencrypt.c
@@ -421,7 +421,7 @@ find_domain_name(struct cifs_ses *ses, const struct nls_table *nls_cp)
if (blobptr + attrsize > blobend)
break;
if (type == NTLMSSP_AV_NB_DOMAIN_NAME) {
- if (!attrsize)
+ if (!attrsize || attrsize >= MAX_CIF_DOMAINNAME)
break;
if (!ses->domainName) {
ses->domainName =
diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
index 33f17ed..88280e0 100644
--- a/fs/cifs/cifsglob.h
+++ b/fs/cifs/cifsglob.h
@@ -396,6 +396,7 @@ struct smb_version_values {
#define HEADER_SIZE(server) (server->vals->header_size)
#define MAX_HEADER_SIZE(server) (server->vals->max_header_size)
+#define MAX_CIF_DOMAINNAME 256 /* 255 + '\0' */
struct smb_vol {
char *username;
diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
index fa68813..ed6bf20 100644
--- a/fs/cifs/connect.c
+++ b/fs/cifs/connect.c
@@ -1675,7 +1675,8 @@ cifs_parse_mount_options(const char *mountdata, const char *devname,
if (string == NULL)
goto out_nomem;
- if (strnlen(string, 256) == 256) {
+ if (strnlen(string, MAX_CIF_DOMAINNAME)
+ == MAX_CIF_DOMAINNAME) {
printk(KERN_WARNING "CIFS: domain name too"
" long\n");
goto cifs_parse_mount_err;
@@ -2276,8 +2277,8 @@ cifs_put_smb_ses(struct cifs_ses *ses)
#ifdef CONFIG_KEYS
-/* strlen("cifs:a:") + INET6_ADDRSTRLEN + 1 */
-#define CIFSCREDS_DESC_SIZE (7 + INET6_ADDRSTRLEN + 1)
+/* strlen("cifs:a:") + MAX_CIF_DOMAINNAME + 1 */
+#define CIFSCREDS_DESC_SIZE (7 + MAX_CIF_DOMAINNAME + 1)
/* Populate username and pw fields from keyring if possible */
static int
diff --git a/fs/cifs/sess.c b/fs/cifs/sess.c
index 79358e3..106a575 100644
--- a/fs/cifs/sess.c
+++ b/fs/cifs/sess.c
@@ -197,7 +197,7 @@ static void unicode_domain_string(char **pbcc_area, struct cifs_ses *ses,
bytes_ret = 0;
} else
bytes_ret = cifs_strtoUTF16((__le16 *) bcc_ptr, ses->domainName,
- 256, nls_cp);
+ MAX_CIF_DOMAINNAME, nls_cp);
bcc_ptr += 2 * bytes_ret;
bcc_ptr += 2; /* account for null terminator */
@@ -255,8 +255,8 @@ static void ascii_ssetup_strings(char **pbcc_area, struct cifs_ses *ses,
/* copy domain */
if (ses->domainName != NULL) {
- strncpy(bcc_ptr, ses->domainName, 256);
- bcc_ptr += strnlen(ses->domainName, 256);
+ strncpy(bcc_ptr, ses->domainName, MAX_CIF_DOMAINNAME);
+ bcc_ptr += strnlen(ses->domainName, MAX_CIF_DOMAINNAME);
} /* else we will send a null domain name
so the server will default to its own domain */
*bcc_ptr = 0;
--
1.7.7.6
--
To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
More majordomo info at http://vger.kernel.org/majordomo-info.html

[Steve French]

I assumed it was an 80 column thing - but don't know.
CIFS instead of CIF would be the normal

On Tue, Jul 16, 2013 at 10:47 PM, Shirish Pargaonkar

Post by Shirish Pargaonkar
nitpicking...
Should it be MAX_CIFS_DOMAINNAME instead of MAX_CIF_DOMAINNAME,
unless CIF is short for something here?
Regards,
Shirish

Post by Chen Gang
For cifs_set_cifscreds() in "fs/cifs/connect.c", 'desc' buffer length
is 'CIFSCREDS_DESC_SIZE' (56 is less than 256), and 'ses->domainName'
length may be "255 + '\0'".
The related sprintf() may cause memory overflow, so need extend related
buffer enough to hold all things.
It is also necessary to be sure of 'ses->domainName' must be less than
256, and define the related macro instead of hard code number '256'.
---
fs/cifs/cifsencrypt.c | 2 +-
fs/cifs/cifsglob.h | 1 +
fs/cifs/connect.c | 7 ++++---
fs/cifs/sess.c | 6 +++---
4 files changed, 9 insertions(+), 7 deletions(-)
diff --git a/fs/cifs/cifsencrypt.c b/fs/cifs/cifsencrypt.c
index 45e57cc..ce2d331 100644
--- a/fs/cifs/cifsencrypt.c
+++ b/fs/cifs/cifsencrypt.c
@@ -421,7 +421,7 @@ find_domain_name(struct cifs_ses *ses, const struct nls_table *nls_cp)
if (blobptr + attrsize > blobend)
break;
if (type == NTLMSSP_AV_NB_DOMAIN_NAME) {
- if (!attrsize)
+ if (!attrsize || attrsize >= MAX_CIF_DOMAINNAME)
break;
if (!ses->domainName) {
ses->domainName =
diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
index 33f17ed..88280e0 100644
--- a/fs/cifs/cifsglob.h
+++ b/fs/cifs/cifsglob.h
@@ -396,6 +396,7 @@ struct smb_version_values {
#define HEADER_SIZE(server) (server->vals->header_size)
#define MAX_HEADER_SIZE(server) (server->vals->max_header_size)
+#define MAX_CIF_DOMAINNAME 256 /* 255 + '\0' */
struct smb_vol {
char *username;
diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
index fa68813..ed6bf20 100644
--- a/fs/cifs/connect.c
+++ b/fs/cifs/connect.c
@@ -1675,7 +1675,8 @@ cifs_parse_mount_options(const char *mountdata, const char *devname,
if (string == NULL)
goto out_nomem;
- if (strnlen(string, 256) == 256) {
+ if (strnlen(string, MAX_CIF_DOMAINNAME)
+ == MAX_CIF_DOMAINNAME) {
printk(KERN_WARNING "CIFS: domain name too"
" long\n");
goto cifs_parse_mount_err;
@@ -2276,8 +2277,8 @@ cifs_put_smb_ses(struct cifs_ses *ses)
#ifdef CONFIG_KEYS
-/* strlen("cifs:a:") + INET6_ADDRSTRLEN + 1 */
-#define CIFSCREDS_DESC_SIZE (7 + INET6_ADDRSTRLEN + 1)
+/* strlen("cifs:a:") + MAX_CIF_DOMAINNAME + 1 */
+#define CIFSCREDS_DESC_SIZE (7 + MAX_CIF_DOMAINNAME + 1)
/* Populate username and pw fields from keyring if possible */
static int
diff --git a/fs/cifs/sess.c b/fs/cifs/sess.c
index 79358e3..106a575 100644
--- a/fs/cifs/sess.c
+++ b/fs/cifs/sess.c
@@ -197,7 +197,7 @@ static void unicode_domain_string(char **pbcc_area, struct cifs_ses *ses,
bytes_ret = 0;
} else
bytes_ret = cifs_strtoUTF16((__le16 *) bcc_ptr, ses->domainName,
- 256, nls_cp);
+ MAX_CIF_DOMAINNAME, nls_cp);
bcc_ptr += 2 * bytes_ret;
bcc_ptr += 2; /* account for null terminator */
@@ -255,8 +255,8 @@ static void ascii_ssetup_strings(char **pbcc_area, struct cifs_ses *ses,
/* copy domain */
if (ses->domainName != NULL) {
- strncpy(bcc_ptr, ses->domainName, 256);
- bcc_ptr += strnlen(ses->domainName, 256);
+ strncpy(bcc_ptr, ses->domainName, MAX_CIF_DOMAINNAME);
+ bcc_ptr += strnlen(ses->domainName, MAX_CIF_DOMAINNAME);
} /* else we will send a null domain name
so the server will default to its own domain */
*bcc_ptr = 0;
--
1.7.7.6
--
To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
More majordomo info at http://vger.kernel.org/majordomo-info.html

[Chen Gang]

Post by Shirish Pargaonkar
nitpicking...
Should it be MAX_CIFS_DOMAINNAME instead of MAX_CIF_DOMAINNAME,
unless CIF is short for something here?

Oh, it is my typo mistake, it should be MAX_CIFS_DOMAINNAME.

I will wait for a day to get further checking, if OK, I should send
patch v2 for it.

Thanks.

Post by Shirish Pargaonkar
Regards,
Shirish

Post by Chen Gang
For cifs_set_cifscreds() in "fs/cifs/connect.c", 'desc' buffer length
is 'CIFSCREDS_DESC_SIZE' (56 is less than 256), and 'ses->domainName'
length may be "255 + '\0'".
The related sprintf() may cause memory overflow, so need extend related
buffer enough to hold all things.
It is also necessary to be sure of 'ses->domainName' must be less than
256, and define the related macro instead of hard code number '256'.
---
fs/cifs/cifsencrypt.c | 2 +-
fs/cifs/cifsglob.h | 1 +
fs/cifs/connect.c | 7 ++++---
fs/cifs/sess.c | 6 +++---
4 files changed, 9 insertions(+), 7 deletions(-)
diff --git a/fs/cifs/cifsencrypt.c b/fs/cifs/cifsencrypt.c
index 45e57cc..ce2d331 100644
--- a/fs/cifs/cifsencrypt.c
+++ b/fs/cifs/cifsencrypt.c
@@ -421,7 +421,7 @@ find_domain_name(struct cifs_ses *ses, const struct nls_table *nls_cp)
if (blobptr + attrsize > blobend)
break;
if (type == NTLMSSP_AV_NB_DOMAIN_NAME) {
- if (!attrsize)
+ if (!attrsize || attrsize >= MAX_CIF_DOMAINNAME)
break;
if (!ses->domainName) {
ses->domainName =
diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
index 33f17ed..88280e0 100644
--- a/fs/cifs/cifsglob.h
+++ b/fs/cifs/cifsglob.h
@@ -396,6 +396,7 @@ struct smb_version_values {
#define HEADER_SIZE(server) (server->vals->header_size)
#define MAX_HEADER_SIZE(server) (server->vals->max_header_size)
+#define MAX_CIF_DOMAINNAME 256 /* 255 + '\0' */
struct smb_vol {
char *username;
diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
index fa68813..ed6bf20 100644
--- a/fs/cifs/connect.c
+++ b/fs/cifs/connect.c
@@ -1675,7 +1675,8 @@ cifs_parse_mount_options(const char *mountdata, const char *devname,
if (string == NULL)
goto out_nomem;
- if (strnlen(string, 256) == 256) {
+ if (strnlen(string, MAX_CIF_DOMAINNAME)
+ == MAX_CIF_DOMAINNAME) {
printk(KERN_WARNING "CIFS: domain name too"
" long\n");
goto cifs_parse_mount_err;
@@ -2276,8 +2277,8 @@ cifs_put_smb_ses(struct cifs_ses *ses)
#ifdef CONFIG_KEYS
-/* strlen("cifs:a:") + INET6_ADDRSTRLEN + 1 */
-#define CIFSCREDS_DESC_SIZE (7 + INET6_ADDRSTRLEN + 1)
+/* strlen("cifs:a:") + MAX_CIF_DOMAINNAME + 1 */
+#define CIFSCREDS_DESC_SIZE (7 + MAX_CIF_DOMAINNAME + 1)
/* Populate username and pw fields from keyring if possible */
static int
diff --git a/fs/cifs/sess.c b/fs/cifs/sess.c
index 79358e3..106a575 100644
--- a/fs/cifs/sess.c
+++ b/fs/cifs/sess.c
@@ -197,7 +197,7 @@ static void unicode_domain_string(char **pbcc_area, struct cifs_ses *ses,
bytes_ret = 0;
} else
bytes_ret = cifs_strtoUTF16((__le16 *) bcc_ptr, ses->domainName,
- 256, nls_cp);
+ MAX_CIF_DOMAINNAME, nls_cp);
bcc_ptr += 2 * bytes_ret;
bcc_ptr += 2; /* account for null terminator */
@@ -255,8 +255,8 @@ static void ascii_ssetup_strings(char **pbcc_area, struct cifs_ses *ses,
/* copy domain */
if (ses->domainName != NULL) {
- strncpy(bcc_ptr, ses->domainName, 256);
- bcc_ptr += strnlen(ses->domainName, 256);
+ strncpy(bcc_ptr, ses->domainName, MAX_CIF_DOMAINNAME);
+ bcc_ptr += strnlen(ses->domainName, MAX_CIF_DOMAINNAME);
} /* else we will send a null domain name
so the server will default to its own domain */
*bcc_ptr = 0;
--
1.7.7.6
--
To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
More majordomo info at http://vger.kernel.org/majordomo-info.html

[Jeff Layton]

On Tue, 16 Jul 2013 22:47:35 -0500

Post by Shirish Pargaonkar
nitpicking...
Should it be MAX_CIFS_DOMAINNAME instead of MAX_CIF_DOMAINNAME,
unless CIF is short for something here?
Regards,
Shirish

Even better...

We already have a MAX_USERNAME_SIZE. Maybe call it MAX_DOMAINNAME_SIZE
for parity with that? Might also want to relocate the #define next to
that one since it would be helpful to have all of those lengths grouped
in the same place.

Post by Shirish Pargaonkar

Post by Chen Gang
For cifs_set_cifscreds() in "fs/cifs/connect.c", 'desc' buffer length
is 'CIFSCREDS_DESC_SIZE' (56 is less than 256), and 'ses->domainName'
length may be "255 + '\0'".
The related sprintf() may cause memory overflow, so need extend related
buffer enough to hold all things.

Good catch...

Maybe it would be good to go ahead and turn that sprintf() into a
snprintf() too?

Post by Shirish Pargaonkar

Post by Chen Gang
It is also necessary to be sure of 'ses->domainName' must be less than
256, and define the related macro instead of hard code number '256'.
---
fs/cifs/cifsencrypt.c | 2 +-
fs/cifs/cifsglob.h | 1 +
fs/cifs/connect.c | 7 ++++---
fs/cifs/sess.c | 6 +++---
4 files changed, 9 insertions(+), 7 deletions(-)
diff --git a/fs/cifs/cifsencrypt.c b/fs/cifs/cifsencrypt.c
index 45e57cc..ce2d331 100644
--- a/fs/cifs/cifsencrypt.c
+++ b/fs/cifs/cifsencrypt.c
@@ -421,7 +421,7 @@ find_domain_name(struct cifs_ses *ses, const struct nls_table *nls_cp)
if (blobptr + attrsize > blobend)
break;
if (type == NTLMSSP_AV_NB_DOMAIN_NAME) {
- if (!attrsize)
+ if (!attrsize || attrsize >= MAX_CIF_DOMAINNAME)
break;
if (!ses->domainName) {
ses->domainName =
diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
index 33f17ed..88280e0 100644
--- a/fs/cifs/cifsglob.h
+++ b/fs/cifs/cifsglob.h
@@ -396,6 +396,7 @@ struct smb_version_values {
#define HEADER_SIZE(server) (server->vals->header_size)
#define MAX_HEADER_SIZE(server) (server->vals->max_header_size)
+#define MAX_CIF_DOMAINNAME 256 /* 255 + '\0' */
struct smb_vol {
char *username;
diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
index fa68813..ed6bf20 100644
--- a/fs/cifs/connect.c
+++ b/fs/cifs/connect.c
@@ -1675,7 +1675,8 @@ cifs_parse_mount_options(const char *mountdata, const char *devname,
if (string == NULL)
goto out_nomem;
- if (strnlen(string, 256) == 256) {
+ if (strnlen(string, MAX_CIF_DOMAINNAME)
+ == MAX_CIF_DOMAINNAME) {
printk(KERN_WARNING "CIFS: domain name too"
" long\n");
goto cifs_parse_mount_err;
@@ -2276,8 +2277,8 @@ cifs_put_smb_ses(struct cifs_ses *ses)
#ifdef CONFIG_KEYS
-/* strlen("cifs:a:") + INET6_ADDRSTRLEN + 1 */
-#define CIFSCREDS_DESC_SIZE (7 + INET6_ADDRSTRLEN + 1)
+/* strlen("cifs:a:") + MAX_CIF_DOMAINNAME + 1 */
+#define CIFSCREDS_DESC_SIZE (7 + MAX_CIF_DOMAINNAME + 1)
/* Populate username and pw fields from keyring if possible */
static int
diff --git a/fs/cifs/sess.c b/fs/cifs/sess.c
index 79358e3..106a575 100644
--- a/fs/cifs/sess.c
+++ b/fs/cifs/sess.c
@@ -197,7 +197,7 @@ static void unicode_domain_string(char **pbcc_area, struct cifs_ses *ses,
bytes_ret = 0;
} else
bytes_ret = cifs_strtoUTF16((__le16 *) bcc_ptr, ses->domainName,
- 256, nls_cp);
+ MAX_CIF_DOMAINNAME, nls_cp);
bcc_ptr += 2 * bytes_ret;
bcc_ptr += 2; /* account for null terminator */
@@ -255,8 +255,8 @@ static void ascii_ssetup_strings(char **pbcc_area, struct cifs_ses *ses,
/* copy domain */
if (ses->domainName != NULL) {
- strncpy(bcc_ptr, ses->domainName, 256);
- bcc_ptr += strnlen(ses->domainName, 256);
+ strncpy(bcc_ptr, ses->domainName, MAX_CIF_DOMAINNAME);
+ bcc_ptr += strnlen(ses->domainName, MAX_CIF_DOMAINNAME);
} /* else we will send a null domain name
so the server will default to its own domain */
*bcc_ptr = 0;
--
1.7.7.6
--
To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
More majordomo info at http://vger.kernel.org/majordomo-info.html

--
To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
More majordomo info at http://vger.kernel.org/majordomo-info.html

[Chen Gang]

Post by Jeff Layton
On Tue, 16 Jul 2013 22:47:35 -0500

Post by Shirish Pargaonkar
nitpicking...
Should it be MAX_CIFS_DOMAINNAME instead of MAX_CIF_DOMAINNAME,
unless CIF is short for something here?
Regards,
Shirish

Even better...
We already have a MAX_USERNAME_SIZE. Maybe call it MAX_DOMAINNAME_SIZE
for parity with that? Might also want to relocate the #define next to
that one since it would be helpful to have all of those lengths grouped
in the same place.

It sounds reasonable: use MAX_DOMAINNAME_SIZE instead of
MAX_CIFS_DOMAINNAME.

Post by Jeff Layton

Post by Shirish Pargaonkar

Post by Chen Gang
For cifs_set_cifscreds() in "fs/cifs/connect.c", 'desc' buffer length
is 'CIFSCREDS_DESC_SIZE' (56 is less than 256), and 'ses->domainName'
length may be "255 + '\0'".
The related sprintf() may cause memory overflow, so need extend related
buffer enough to hold all things.

Good catch...
Maybe it would be good to go ahead and turn that sprintf() into a
snprintf() too?

Hmm... sprintf() declares to code readers, in current condition, we want
to print all source information without any truncation.

So if we know the source max length precisely, we'd better to allocate
the related buffer to print them all instead of use snprintf().

If we do not know the source max length precisely or we have to limit
the destination length, we need use snprintf() to fit with destination
max length (declare to the code readers, the source information may be
truncated).


Thanks.

Post by Jeff Layton

Post by Shirish Pargaonkar

Post by Chen Gang
It is also necessary to be sure of 'ses->domainName' must be less than
256, and define the related macro instead of hard code number '256'.
---
fs/cifs/cifsencrypt.c | 2 +-
fs/cifs/cifsglob.h | 1 +
fs/cifs/connect.c | 7 ++++---
fs/cifs/sess.c | 6 +++---
4 files changed, 9 insertions(+), 7 deletions(-)
diff --git a/fs/cifs/cifsencrypt.c b/fs/cifs/cifsencrypt.c
index 45e57cc..ce2d331 100644
--- a/fs/cifs/cifsencrypt.c
+++ b/fs/cifs/cifsencrypt.c
@@ -421,7 +421,7 @@ find_domain_name(struct cifs_ses *ses, const struct nls_table *nls_cp)
if (blobptr + attrsize > blobend)
break;
if (type == NTLMSSP_AV_NB_DOMAIN_NAME) {
- if (!attrsize)
+ if (!attrsize || attrsize >= MAX_CIF_DOMAINNAME)
break;
if (!ses->domainName) {
ses->domainName =
diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
index 33f17ed..88280e0 100644
--- a/fs/cifs/cifsglob.h
+++ b/fs/cifs/cifsglob.h
@@ -396,6 +396,7 @@ struct smb_version_values {
#define HEADER_SIZE(server) (server->vals->header_size)
#define MAX_HEADER_SIZE(server) (server->vals->max_header_size)
+#define MAX_CIF_DOMAINNAME 256 /* 255 + '\0' */
struct smb_vol {
char *username;
diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
index fa68813..ed6bf20 100644
--- a/fs/cifs/connect.c
+++ b/fs/cifs/connect.c
@@ -1675,7 +1675,8 @@ cifs_parse_mount_options(const char *mountdata, const char *devname,
if (string == NULL)
goto out_nomem;
- if (strnlen(string, 256) == 256) {
+ if (strnlen(string, MAX_CIF_DOMAINNAME)
+ == MAX_CIF_DOMAINNAME) {
printk(KERN_WARNING "CIFS: domain name too"
" long\n");
goto cifs_parse_mount_err;
@@ -2276,8 +2277,8 @@ cifs_put_smb_ses(struct cifs_ses *ses)
#ifdef CONFIG_KEYS
-/* strlen("cifs:a:") + INET6_ADDRSTRLEN + 1 */
-#define CIFSCREDS_DESC_SIZE (7 + INET6_ADDRSTRLEN + 1)
+/* strlen("cifs:a:") + MAX_CIF_DOMAINNAME + 1 */
+#define CIFSCREDS_DESC_SIZE (7 + MAX_CIF_DOMAINNAME + 1)
/* Populate username and pw fields from keyring if possible */
static int
diff --git a/fs/cifs/sess.c b/fs/cifs/sess.c
index 79358e3..106a575 100644
--- a/fs/cifs/sess.c
+++ b/fs/cifs/sess.c
@@ -197,7 +197,7 @@ static void unicode_domain_string(char **pbcc_area, struct cifs_ses *ses,
bytes_ret = 0;
} else
bytes_ret = cifs_strtoUTF16((__le16 *) bcc_ptr, ses->domainName,
- 256, nls_cp);
+ MAX_CIF_DOMAINNAME, nls_cp);
bcc_ptr += 2 * bytes_ret;
bcc_ptr += 2; /* account for null terminator */
@@ -255,8 +255,8 @@ static void ascii_ssetup_strings(char **pbcc_area, struct cifs_ses *ses,
/* copy domain */
if (ses->domainName != NULL) {
- strncpy(bcc_ptr, ses->domainName, 256);
- bcc_ptr += strnlen(ses->domainName, 256);
+ strncpy(bcc_ptr, ses->domainName, MAX_CIF_DOMAINNAME);
+ bcc_ptr += strnlen(ses->domainName, MAX_CIF_DOMAINNAME);
} /* else we will send a null domain name
so the server will default to its own domain */
*bcc_ptr = 0;
--
1.7.7.6
--
To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
More majordomo info at http://vger.kernel.org/majordomo-info.html

--
To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
More majordomo info at http://vger.kernel.org/majordomo-info.html

[Jeff Layton]

On Thu, 18 Jul 2013 09:04:30 +0800

Post by Chen Gang

Post by Jeff Layton
On Tue, 16 Jul 2013 22:47:35 -0500

Post by Shirish Pargaonkar
nitpicking...
Should it be MAX_CIFS_DOMAINNAME instead of MAX_CIF_DOMAINNAME,
unless CIF is short for something here?
Regards,
Shirish

Even better...
We already have a MAX_USERNAME_SIZE. Maybe call it MAX_DOMAINNAME_SIZE
for parity with that? Might also want to relocate the #define next to
that one since it would be helpful to have all of those lengths grouped
in the same place.

It sounds reasonable: use MAX_DOMAINNAME_SIZE instead of
MAX_CIFS_DOMAINNAME.

Post by Jeff Layton

Post by Shirish Pargaonkar

Post by Chen Gang
For cifs_set_cifscreds() in "fs/cifs/connect.c", 'desc' buffer length
is 'CIFSCREDS_DESC_SIZE' (56 is less than 256), and 'ses->domainName'
length may be "255 + '\0'".
The related sprintf() may cause memory overflow, so need extend related
buffer enough to hold all things.

Good catch...
Maybe it would be good to go ahead and turn that sprintf() into a
snprintf() too?

Hmm... sprintf() declares to code readers, in current condition, we want
to print all source information without any truncation.
So if we know the source max length precisely, we'd better to allocate
the related buffer to print them all instead of use snprintf().
If we do not know the source max length precisely or we have to limit
the destination length, we need use snprintf() to fit with destination
max length (declare to the code readers, the source information may be
truncated).

Fair enough. It was more of a suggestion for defensive coding. But
since we know the length of both buffers and that the source is NULL
terminated, then sprintf is fine.

Patch looks fine to me otherwise.

Acked-by: Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+***@public.gmane.org>

[Chen Gang]

Post by Jeff Layton
On Thu, 18 Jul 2013 09:04:30 +0800

Post by Chen Gang

Post by Jeff Layton
On Tue, 16 Jul 2013 22:47:35 -0500

Post by Shirish Pargaonkar
nitpicking...
Should it be MAX_CIFS_DOMAINNAME instead of MAX_CIF_DOMAINNAME,
unless CIF is short for something here?
Regards,
Shirish

Even better...
We already have a MAX_USERNAME_SIZE. Maybe call it MAX_DOMAINNAME_SIZE
for parity with that? Might also want to relocate the #define next to
that one since it would be helpful to have all of those lengths grouped
in the same place.

It sounds reasonable: use MAX_DOMAINNAME_SIZE instead of
MAX_CIFS_DOMAINNAME.

Post by Jeff Layton

Post by Shirish Pargaonkar

Post by Chen Gang
For cifs_set_cifscreds() in "fs/cifs/connect.c", 'desc' buffer length
is 'CIFSCREDS_DESC_SIZE' (56 is less than 256), and 'ses->domainName'
length may be "255 + '\0'".
The related sprintf() may cause memory overflow, so need extend related
buffer enough to hold all things.

Good catch...
Maybe it would be good to go ahead and turn that sprintf() into a
snprintf() too?

Hmm... sprintf() declares to code readers, in current condition, we want
to print all source information without any truncation.
So if we know the source max length precisely, we'd better to allocate
the related buffer to print them all instead of use snprintf().
If we do not know the source max length precisely or we have to limit
the destination length, we need use snprintf() to fit with destination
max length (declare to the code readers, the source information may be
truncated).

Fair enough. It was more of a suggestion for defensive coding. But
since we know the length of both buffers and that the source is NULL
terminated, then sprintf is fine.
Patch looks fine to me otherwise.

Thank you for your Acked-by.

If possible, I will/should wait a day, if no another additional
suggestions or completions, I should send patch v2 tomorrow.


Thanks.

[Chen Gang]

Post by Chen Gang

Post by Jeff Layton
Maybe it would be good to go ahead and turn that sprintf() into a
snprintf() too?

Hmm... sprintf() declares to code readers, in current condition, we want
to print all source information without any truncation.
So if we know the source max length precisely, we'd better to allocate
the related buffer to print them all instead of use snprintf().
If we do not know the source max length precisely or we have to limit
the destination length, we need use snprintf() to fit with destination
max length (declare to the code readers, the source information may be
truncated).

My original idea for snprintf() is incorrect, the reason is: "Of course
you would have to check the return value of snprintf() to detect a
truncation and abort..." (Thank Richard).



Thanks.