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

[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+dBDgjK7y7TUQ@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;
-- 
1.7.7.6

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

[Scott Lovenberg]

On Tue, Jul 16, 2013 at 8:48 PM, Chen Gang <gang.chen-bOixZGp5f+dBDgjK7y7TUQ@public.gmane.org> wrote:
> 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+dBDgjK7y7TUQ@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;
> --
> 1.7.7.6
> --
> To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> 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.

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

[Steve French]

On Tue, Jul 16, 2013 at 8:58 PM, Scott Lovenberg
<scott.lovenberg-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> On Tue, Jul 16, 2013 at 8:48 PM, Chen Gang <gang.chen-bOixZGp5f+dBDgjK7y7TUQ@public.gmane.org> wrote:
>> 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+dBDgjK7y7TUQ@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;
>> --
>> 1.7.7.6
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
>> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
>> 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."


-- 
Thanks,

Steve

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

[Chen Gang]

On 07/17/2013 10:06 AM, Steve French wrote:
> On Tue, Jul 16, 2013 at 8:58 PM, Scott Lovenberg
> <scott.lovenberg-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>> > On Tue, Jul 16, 2013 at 8:48 PM, Chen Gang <gang.chen-bOixZGp5f+dBDgjK7y7TUQ@public.gmane.org> wrote:
>>> >> 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+dBDgjK7y7TUQ@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;
>>> >> --
>>> >> 1.7.7.6
>>> >> --
>>> >> To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
>>> >> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
>>> >> 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.

-- 
Chen Gang

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

[Scott Lovenberg]

On Tue, Jul 16, 2013 at 10:06 PM, Steve French <smfrench-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:

>> 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.

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

[Chen Gang]

On 07/18/2013 02:27 AM, Scott Lovenberg wrote:
> On Tue, Jul 16, 2013 at 10:06 PM, Steve French <smfrench-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> 
>>> 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 ?

> 
> --
> Peace and Blessings,
> -Scott.
> 
> 

Thanks.
-- 
Chen Gang

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

[Scott Lovenberg]

On Wed, Jul 17, 2013 at 9:08 PM, Chen Gang <gang.chen@asianux.com> wrote:
> On 07/18/2013 02:27 AM, Scott Lovenberg wrote:
>> On Tue, Jul 16, 2013 at 10:06 PM, Steve French <smfrench@gmail.com> wrote:
>>
>>>> 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.

[PATCH v2] 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+dBDgjK7y7TUQ@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;
-- 
1.7.7.6

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

[Jeff Layton]

On Fri, 19 Jul 2013 09:01:36 +0800
Chen Gang <gang.chen-bOixZGp5f+dBDgjK7y7TUQ@public.gmane.org> wrote:

> 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+dBDgjK7y7TUQ@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;

Looks good...

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

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

[Steve French]

merged into cifs-2.6.git

On Fri, Jul 19, 2013 at 5:45 AM, Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
> On Fri, 19 Jul 2013 09:01:36 +0800
> Chen Gang <gang.chen-bOixZGp5f+dBDgjK7y7TUQ@public.gmane.org> wrote:
>
>> 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+dBDgjK7y7TUQ@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;
>
> Looks good...
>
> Reviewed-by: Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>



-- 
Thanks,

Steve

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

[Steve French]

Added CC: <stable-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>

On Fri, Jul 19, 2013 at 10:46 AM, Steve French <smfrench-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> merged into cifs-2.6.git
>
> On Fri, Jul 19, 2013 at 5:45 AM, Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
>> On Fri, 19 Jul 2013 09:01:36 +0800
>> Chen Gang <gang.chen-bOixZGp5f+dBDgjK7y7TUQ@public.gmane.org> wrote:
>>
>>> 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+dBDgjK7y7TUQ@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;
>>
>> Looks good...
>>
>> Reviewed-by: Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
>
>
>
> --
> Thanks,
>
> Steve



-- 
Thanks,

Steve

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

[Chen Gang]

On 07/19/2013 11:46 PM, Steve French wrote:
> merged into cifs-2.6.git
> 

Thanks.

> On Fri, Jul 19, 2013 at 5:45 AM, Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
>> > On Fri, 19 Jul 2013 09:01:36 +0800
>> > Chen Gang <gang.chen-bOixZGp5f+dBDgjK7y7TUQ@public.gmane.org> wrote:
>> >
>>> >> 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+dBDgjK7y7TUQ@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;
>> >
>> > Looks good...
>> >
>> > Reviewed-by: Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> 
> 
> -- Thanks, Steve
> 


-- 
Chen Gang

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

[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+dBDgjK7y7TUQ@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+dBDgjK7y7TUQ@public.gmane.org>
    Reviewed-by: Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
    Reviewed-by: Shirish Pargaonkar <shirishpargaonkar-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
    Reviewed-by: Scott Lovenberg <scott.lovenberg-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
    CC: <stable-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>
    Signed-off-by: Steve French <smfrench-Re5JQEeQqe8AvxtiuMwx3w@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;

On Sun, Jul 21, 2013 at 8:21 PM, Chen Gang <gang.chen-bOixZGp5f+dBDgjK7y7TUQ@public.gmane.org> wrote:
> On 07/19/2013 11:46 PM, Steve French wrote:
>> merged into cifs-2.6.git
>>
>
> Thanks.
>
>> On Fri, Jul 19, 2013 at 5:45 AM, Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
>>> > On Fri, 19 Jul 2013 09:01:36 +0800
>>> > Chen Gang <gang.chen-bOixZGp5f+dBDgjK7y7TUQ@public.gmane.org> wrote:
>>> >
>>>> >> 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+dBDgjK7y7TUQ@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;
>>> >
>>> > Looks good...
>>> >
>>> > Reviewed-by: Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
>>
>>
>> -- Thanks, Steve
>>
>
>
> --
> Chen Gang



-- 
Thanks,

Steve

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

[Scott Lovenberg]

On Wed, Jul 31, 2013 at 12:59 AM, Steve French <smfrench-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> 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.

-- 
Peace and Blessings,
-Scott.

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

[Chen Gang]

On 07/19/2013 06:45 PM, Jeff Layton wrote:
> On Fri, 19 Jul 2013 09:01:36 +0800
> Chen Gang <gang.chen-bOixZGp5f+dBDgjK7y7TUQ@public.gmane.org> wrote:
> 
>> 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+dBDgjK7y7TUQ@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;
> 
> Looks good...
> 
> Reviewed-by: Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> 
> 

Thanks.

-- 
Chen Gang

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

[Chen Gang]

On 07/17/2013 09:58 AM, Scott Lovenberg wrote:
> On Tue, Jul 16, 2013 at 8:48 PM, Chen Gang <gang.chen-bOixZGp5f+dBDgjK7y7TUQ@public.gmane.org> wrote:
>> > 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+dBDgjK7y7TUQ@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;
>> > --
>> > 1.7.7.6
>> > --
>> > To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
>> > the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
>> > 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.

-- 
Chen Gang

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

[Jeff Layton]

On Tue, 16 Jul 2013 21:58:19 -0400
Scott Lovenberg <scott.lovenberg-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:

> On Tue, Jul 16, 2013 at 8:48 PM, Chen Gang <gang.chen-bOixZGp5f+dBDgjK7y7TUQ@public.gmane.org> wrote:
> > 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+dBDgjK7y7TUQ@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;
> > --
> > 1.7.7.6
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
> > the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> > 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.

-- 
Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>

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

[Scott Lovenberg]

On Fri, Jul 19, 2013 at 1:51 PM, Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
>
> On Tue, 16 Jul 2013 21:58:19 -0400
> Scott Lovenberg <scott.lovenberg-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>
> > On Tue, Jul 16, 2013 at 8:48 PM, Chen Gang <gang.chen-bOixZGp5f+dBDgjK7y7TUQ@public.gmane.org> wrote:
> > > 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+dBDgjK7y7TUQ@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;
> > > --
> > > 1.7.7.6
> > > --
> > > To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
> > > the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> > > 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.
>
> --
> Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>


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.

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

[Jeff Layton]

On Fri, 19 Jul 2013 15:32:29 -0400
Scott Lovenberg <scott.lovenberg-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:

> On Fri, Jul 19, 2013 at 1:51 PM, Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
> >
> > On Tue, 16 Jul 2013 21:58:19 -0400
> > Scott Lovenberg <scott.lovenberg-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> >
> > > On Tue, Jul 16, 2013 at 8:48 PM, Chen Gang <gang.chen-bOixZGp5f+dBDgjK7y7TUQ@public.gmane.org> wrote:
> > > > 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+dBDgjK7y7TUQ@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;
> > > > --
> > > > 1.7.7.6
> > > > --
> > > > To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
> > > > the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> > > > 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.
> >
> > --
> > Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> 
> 
> 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...

-- 
Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>

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

[Shirish Pargaonkar]

nitpicking...

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

Regards,

Shirish

On Tue, Jul 16, 2013 at 7:48 PM, Chen Gang <gang.chen-bOixZGp5f+dBDgjK7y7TUQ@public.gmane.org> wrote:
> 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+dBDgjK7y7TUQ@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;
> --
> 1.7.7.6
> --
> To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[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
<shirishpargaonkar-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> nitpicking...
>
> Should it be MAX_CIFS_DOMAINNAME instead of MAX_CIF_DOMAINNAME,
> unless CIF is short for something here?
>
> Regards,
>
> Shirish
>
> On Tue, Jul 16, 2013 at 7:48 PM, Chen Gang <gang.chen-bOixZGp5f+dBDgjK7y7TUQ@public.gmane.org> wrote:
>> 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+dBDgjK7y7TUQ@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;
>> --
>> 1.7.7.6
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
>> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html



-- 
Thanks,

Steve

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

[Chen Gang]

On 07/17/2013 11:47 AM, Shirish Pargaonkar wrote:
> 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.

> Regards,
> 
> Shirish
> 
> On Tue, Jul 16, 2013 at 7:48 PM, Chen Gang <gang.chen-bOixZGp5f+dBDgjK7y7TUQ@public.gmane.org> wrote:
>> 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+dBDgjK7y7TUQ@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;
>> --
>> 1.7.7.6
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
>> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
> 


-- 
Chen Gang

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

[Jeff Layton]

On Tue, 16 Jul 2013 22:47:35 -0500
Shirish Pargaonkar <shirishpargaonkar-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:

> 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.

> On Tue, Jul 16, 2013 at 7:48 PM, Chen Gang <gang.chen-bOixZGp5f+dBDgjK7y7TUQ@public.gmane.org> wrote:
> > 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?

> > 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+dBDgjK7y7TUQ@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;
> > --
> > 1.7.7.6
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
> > the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> --
> To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html


-- 
Jeff Layton <jlayton-vpEMnDpepFuMZCB2o+C8xQ@public.gmane.org>

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

[Chen Gang]

On 07/17/2013 07:24 PM, Jeff Layton wrote:
> On Tue, 16 Jul 2013 22:47:35 -0500
> Shirish Pargaonkar <shirishpargaonkar-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> 
>> 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.


>> On Tue, Jul 16, 2013 at 7:48 PM, Chen Gang <gang.chen-bOixZGp5f+dBDgjK7y7TUQ@public.gmane.org> wrote:
>>> 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.

>>> 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+dBDgjK7y7TUQ@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;
>>> --
>>> 1.7.7.6
>>> --
>>> To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
>>> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
>> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
> 


-- 
Chen Gang

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

[Jeff Layton]

On Thu, 18 Jul 2013 09:04:30 +0800
Chen Gang <gang.chen-bOixZGp5f+dBDgjK7y7TUQ@public.gmane.org> wrote:

> On 07/17/2013 07:24 PM, Jeff Layton wrote:
> > On Tue, 16 Jul 2013 22:47:35 -0500
> > Shirish Pargaonkar <shirishpargaonkar-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> > 
> >> 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.
> 
> 
> >> On Tue, Jul 16, 2013 at 7:48 PM, Chen Gang <gang.chen-bOixZGp5f+dBDgjK7y7TUQ@public.gmane.org> wrote:
> >>> 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+AlfA@public.gmane.org>

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

[Chen Gang]

On 07/18/2013 09:25 AM, Jeff Layton wrote:
> On Thu, 18 Jul 2013 09:04:30 +0800
> Chen Gang <gang.chen-bOixZGp5f+dBDgjK7y7TUQ@public.gmane.org> wrote:
> 
>> > On 07/17/2013 07:24 PM, Jeff Layton wrote:
>>> > > On Tue, 16 Jul 2013 22:47:35 -0500
>>> > > Shirish Pargaonkar <shirishpargaonkar-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>>> > > 
>>>> > >> 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.
>> > 
>> > 
>>>> > >> On Tue, Jul 16, 2013 at 7:48 PM, Chen Gang <gang.chen-bOixZGp5f+dBDgjK7y7TUQ@public.gmane.org> wrote:
>>>>> > >>> 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+AlfA@public.gmane.org>
> 
> 

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

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

[Chen Gang]

On 07/18/2013 09:25 AM, Jeff Layton wrote:
>> > > 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.
-- 
Chen Gang