* [PATCH] cifs: extend the buffer length enought for sprintf() using
@ 2013-07-17 0:48 Chen Gang
[not found] ` <51E5E9DA.8020603-bOixZGp5f+dBDgjK7y7TUQ@public.gmane.org>
0 siblings, 1 reply; 27+ messages in thread
From: Chen Gang @ 2013-07-17 0:48 UTC (permalink / raw)
To: sfrench-eUNUBHrolfbYtjvyW6yDsg
Cc: linux-cifs-u79uwXL29TY76Z2rM5mHXA,
samba-technical-w/Ol4Ecudpl8XjKLYN78aQ
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
^ permalink raw reply related [flat|nested] 27+ messages in thread
* Re: [PATCH] cifs: extend the buffer length enought for sprintf() using
[not found] ` <51E5E9DA.8020603-bOixZGp5f+dBDgjK7y7TUQ@public.gmane.org>
@ 2013-07-17 1:58 ` Scott Lovenberg
[not found] ` <CAFB9KM2nJEt-O+o=4bkxNMJ2Fr0TfjkpRF=7B98=Lp9zFu8_og-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2013-07-17 3:47 ` Shirish Pargaonkar
1 sibling, 1 reply; 27+ messages in thread
From: Scott Lovenberg @ 2013-07-17 1:58 UTC (permalink / raw)
To: Chen Gang, Jeff Layton
Cc: sfrench-eUNUBHrolfbYtjvyW6yDsg, linux-cifs,
samba-technical-w/Ol4Ecudpl8XjKLYN78aQ
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.
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH] cifs: extend the buffer length enought for sprintf() using
[not found] ` <CAFB9KM2nJEt-O+o=4bkxNMJ2Fr0TfjkpRF=7B98=Lp9zFu8_og-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
@ 2013-07-17 2:06 ` Steve French
[not found] ` <CAH2r5msQEbQWpE+wqEoLY_++=ywDVoAg_vmWB3kJG8=ECHC3Pg-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2013-07-17 2:07 ` [PATCH] " Chen Gang
2013-07-19 17:51 ` Jeff Layton
2 siblings, 1 reply; 27+ messages in thread
From: Steve French @ 2013-07-17 2:06 UTC (permalink / raw)
To: Scott Lovenberg
Cc: Chen Gang, Jeff Layton, sfrench-eUNUBHrolfbYtjvyW6yDsg,
linux-cifs, samba-technical-w/Ol4Ecudpl8XjKLYN78aQ
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
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH] cifs: extend the buffer length enought for sprintf() using
[not found] ` <CAFB9KM2nJEt-O+o=4bkxNMJ2Fr0TfjkpRF=7B98=Lp9zFu8_og-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2013-07-17 2:06 ` Steve French
@ 2013-07-17 2:07 ` Chen Gang
2013-07-19 17:51 ` Jeff Layton
2 siblings, 0 replies; 27+ messages in thread
From: Chen Gang @ 2013-07-17 2:07 UTC (permalink / raw)
To: Scott Lovenberg
Cc: Jeff Layton, sfrench-eUNUBHrolfbYtjvyW6yDsg, linux-cifs,
samba-technical-w/Ol4Ecudpl8XjKLYN78aQ
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
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH] cifs: extend the buffer length enought for sprintf() using
[not found] ` <CAH2r5msQEbQWpE+wqEoLY_++=ywDVoAg_vmWB3kJG8=ECHC3Pg-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
@ 2013-07-17 2:11 ` Chen Gang
2013-07-17 18:27 ` Scott Lovenberg
1 sibling, 0 replies; 27+ messages in thread
From: Chen Gang @ 2013-07-17 2:11 UTC (permalink / raw)
To: Steve French
Cc: Scott Lovenberg, Jeff Layton, sfrench-eUNUBHrolfbYtjvyW6yDsg,
linux-cifs, samba-technical-w/Ol4Ecudpl8XjKLYN78aQ
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
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH] cifs: extend the buffer length enought for sprintf() using
[not found] ` <51E5E9DA.8020603-bOixZGp5f+dBDgjK7y7TUQ@public.gmane.org>
2013-07-17 1:58 ` Scott Lovenberg
@ 2013-07-17 3:47 ` Shirish Pargaonkar
[not found] ` <CADT32e+Ydg5N8uWyvCKee8n4iS34LcCFgE1nrC2S8bq7GnA-hg-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
1 sibling, 1 reply; 27+ messages in thread
From: Shirish Pargaonkar @ 2013-07-17 3:47 UTC (permalink / raw)
To: Chen Gang; +Cc: Steve French, linux-cifs, samba-technical
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
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH] cifs: extend the buffer length enought for sprintf() using
[not found] ` <CADT32e+Ydg5N8uWyvCKee8n4iS34LcCFgE1nrC2S8bq7GnA-hg-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
@ 2013-07-17 3:54 ` Steve French
2013-07-17 4:21 ` Chen Gang
2013-07-17 11:24 ` Jeff Layton
2 siblings, 0 replies; 27+ messages in thread
From: Steve French @ 2013-07-17 3:54 UTC (permalink / raw)
To: Shirish Pargaonkar; +Cc: Chen Gang, Steve French, linux-cifs, samba-technical
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
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH] cifs: extend the buffer length enought for sprintf() using
[not found] ` <CADT32e+Ydg5N8uWyvCKee8n4iS34LcCFgE1nrC2S8bq7GnA-hg-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2013-07-17 3:54 ` Steve French
@ 2013-07-17 4:21 ` Chen Gang
2013-07-17 11:24 ` Jeff Layton
2 siblings, 0 replies; 27+ messages in thread
From: Chen Gang @ 2013-07-17 4:21 UTC (permalink / raw)
To: Shirish Pargaonkar; +Cc: Steve French, linux-cifs, samba-technical
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
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH] cifs: extend the buffer length enought for sprintf() using
[not found] ` <CADT32e+Ydg5N8uWyvCKee8n4iS34LcCFgE1nrC2S8bq7GnA-hg-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2013-07-17 3:54 ` Steve French
2013-07-17 4:21 ` Chen Gang
@ 2013-07-17 11:24 ` Jeff Layton
[not found] ` <20130717072431.5d8a22b3-9yPaYZwiELC+kQycOl6kW4xkIHaj4LzF@public.gmane.org>
2 siblings, 1 reply; 27+ messages in thread
From: Jeff Layton @ 2013-07-17 11:24 UTC (permalink / raw)
To: Shirish Pargaonkar; +Cc: Chen Gang, Steve French, linux-cifs, samba-technical
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>
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH] cifs: extend the buffer length enought for sprintf() using
[not found] ` <CAH2r5msQEbQWpE+wqEoLY_++=ywDVoAg_vmWB3kJG8=ECHC3Pg-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2013-07-17 2:11 ` Chen Gang
@ 2013-07-17 18:27 ` Scott Lovenberg
[not found] ` <CAFB9KM0rEDyE6hb8t-gDLTDKq9kaRr4Bhs7SLBEZTnyH5u5U-A-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
1 sibling, 1 reply; 27+ messages in thread
From: Scott Lovenberg @ 2013-07-17 18:27 UTC (permalink / raw)
To: Steve French
Cc: Chen Gang, Jeff Layton, sfrench-eUNUBHrolfbYtjvyW6yDsg,
linux-cifs, samba-technical-w/Ol4Ecudpl8XjKLYN78aQ
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.
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH] cifs: extend the buffer length enought for sprintf() using
[not found] ` <20130717072431.5d8a22b3-9yPaYZwiELC+kQycOl6kW4xkIHaj4LzF@public.gmane.org>
@ 2013-07-18 1:04 ` Chen Gang
[not found] ` <51E73F1E.4010804-bOixZGp5f+dBDgjK7y7TUQ@public.gmane.org>
0 siblings, 1 reply; 27+ messages in thread
From: Chen Gang @ 2013-07-18 1:04 UTC (permalink / raw)
To: Jeff Layton; +Cc: Shirish Pargaonkar, Steve French, linux-cifs, samba-technical
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
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH] cifs: extend the buffer length enought for sprintf() using
[not found] ` <CAFB9KM0rEDyE6hb8t-gDLTDKq9kaRr4Bhs7SLBEZTnyH5u5U-A-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
@ 2013-07-18 1:08 ` Chen Gang
2013-07-18 6:47 ` Scott Lovenberg
0 siblings, 1 reply; 27+ messages in thread
From: Chen Gang @ 2013-07-18 1:08 UTC (permalink / raw)
To: Scott Lovenberg
Cc: Steve French, Jeff Layton, sfrench-eUNUBHrolfbYtjvyW6yDsg,
linux-cifs, samba-technical-w/Ol4Ecudpl8XjKLYN78aQ
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
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH] cifs: extend the buffer length enought for sprintf() using
[not found] ` <51E73F1E.4010804-bOixZGp5f+dBDgjK7y7TUQ@public.gmane.org>
@ 2013-07-18 1:25 ` Jeff Layton
[not found] ` <20130717212559.71b7af06-4QP7MXygkU+dMjc06nkz3ljfA9RmPOcC@public.gmane.org>
0 siblings, 1 reply; 27+ messages in thread
From: Jeff Layton @ 2013-07-18 1:25 UTC (permalink / raw)
To: Chen Gang; +Cc: Shirish Pargaonkar, Steve French, linux-cifs, samba-technical
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>
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH] cifs: extend the buffer length enought for sprintf() using
[not found] ` <20130717212559.71b7af06-4QP7MXygkU+dMjc06nkz3ljfA9RmPOcC@public.gmane.org>
@ 2013-07-18 1:31 ` Chen Gang
2013-10-06 0:49 ` Chen Gang
1 sibling, 0 replies; 27+ messages in thread
From: Chen Gang @ 2013-07-18 1:31 UTC (permalink / raw)
To: Jeff Layton; +Cc: Shirish Pargaonkar, Steve French, linux-cifs, samba-technical
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
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH] cifs: extend the buffer length enought for sprintf() using
2013-07-18 1:08 ` Chen Gang
@ 2013-07-18 6:47 ` Scott Lovenberg
[not found] ` <CAFB9KM2MbLuETpoN9wafZLq6B9StjtXnG15G16uGcOcnRM8+sA-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
0 siblings, 1 reply; 27+ messages in thread
From: Scott Lovenberg @ 2013-07-18 6:47 UTC (permalink / raw)
To: Chen Gang; +Cc: sfrench, Steve French, samba-technical, Jeff Layton, linux-cifs
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.
^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH v2] cifs: extend the buffer length enought for sprintf() using
[not found] ` <CAFB9KM2MbLuETpoN9wafZLq6B9StjtXnG15G16uGcOcnRM8+sA-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
@ 2013-07-19 1:01 ` Chen Gang
[not found] ` <51E88FF0.2010101-bOixZGp5f+dBDgjK7y7TUQ@public.gmane.org>
0 siblings, 1 reply; 27+ messages in thread
From: Chen Gang @ 2013-07-19 1:01 UTC (permalink / raw)
To: Scott Lovenberg
Cc: Steve French, Jeff Layton, sfrench-eUNUBHrolfbYtjvyW6yDsg,
linux-cifs, samba-technical-w/Ol4Ecudpl8XjKLYN78aQ,
Shirish Pargaonkar
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
^ permalink raw reply related [flat|nested] 27+ messages in thread
* Re: [PATCH v2] cifs: extend the buffer length enought for sprintf() using
[not found] ` <51E88FF0.2010101-bOixZGp5f+dBDgjK7y7TUQ@public.gmane.org>
@ 2013-07-19 10:45 ` Jeff Layton
[not found] ` <20130719064531.2a9836f5-9yPaYZwiELC+kQycOl6kW4xkIHaj4LzF@public.gmane.org>
0 siblings, 1 reply; 27+ messages in thread
From: Jeff Layton @ 2013-07-19 10:45 UTC (permalink / raw)
To: Chen Gang
Cc: Scott Lovenberg, Steve French, sfrench-eUNUBHrolfbYtjvyW6yDsg,
linux-cifs, samba-technical-w/Ol4Ecudpl8XjKLYN78aQ,
Shirish Pargaonkar
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>
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2] cifs: extend the buffer length enought for sprintf() using
[not found] ` <20130719064531.2a9836f5-9yPaYZwiELC+kQycOl6kW4xkIHaj4LzF@public.gmane.org>
@ 2013-07-19 15:46 ` Steve French
[not found] ` <CAH2r5mvccBQRikYbbUppmbCO1naSOOMJ+wVWxQEQBxhDdmnP_w-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2013-07-22 0:29 ` Chen Gang
1 sibling, 1 reply; 27+ messages in thread
From: Steve French @ 2013-07-19 15:46 UTC (permalink / raw)
To: Jeff Layton
Cc: Chen Gang, Scott Lovenberg, sfrench-eUNUBHrolfbYtjvyW6yDsg,
linux-cifs, samba-technical-w/Ol4Ecudpl8XjKLYN78aQ,
Shirish Pargaonkar
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
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2] cifs: extend the buffer length enought for sprintf() using
[not found] ` <CAH2r5mvccBQRikYbbUppmbCO1naSOOMJ+wVWxQEQBxhDdmnP_w-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
@ 2013-07-19 16:18 ` Steve French
2013-07-22 1:21 ` Chen Gang
1 sibling, 0 replies; 27+ messages in thread
From: Steve French @ 2013-07-19 16:18 UTC (permalink / raw)
To: Jeff Layton
Cc: Chen Gang, Scott Lovenberg, sfrench-eUNUBHrolfbYtjvyW6yDsg,
linux-cifs, samba-technical-w/Ol4Ecudpl8XjKLYN78aQ,
Shirish Pargaonkar
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
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH] cifs: extend the buffer length enought for sprintf() using
[not found] ` <CAFB9KM2nJEt-O+o=4bkxNMJ2Fr0TfjkpRF=7B98=Lp9zFu8_og-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2013-07-17 2:06 ` Steve French
2013-07-17 2:07 ` [PATCH] " Chen Gang
@ 2013-07-19 17:51 ` Jeff Layton
[not found] ` <20130719135115.2ebff0cd-9yPaYZwiELC+kQycOl6kW4xkIHaj4LzF@public.gmane.org>
2 siblings, 1 reply; 27+ messages in thread
From: Jeff Layton @ 2013-07-19 17:51 UTC (permalink / raw)
To: Scott Lovenberg
Cc: Chen Gang, sfrench-eUNUBHrolfbYtjvyW6yDsg, linux-cifs,
samba-technical-w/Ol4Ecudpl8XjKLYN78aQ
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>
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH] cifs: extend the buffer length enought for sprintf() using
[not found] ` <20130719135115.2ebff0cd-9yPaYZwiELC+kQycOl6kW4xkIHaj4LzF@public.gmane.org>
@ 2013-07-19 19:32 ` Scott Lovenberg
[not found] ` <CAFB9KM1cP1rvnO66+9kz+zVhVOf+C-dAR7mVMx_uX9hT-ORLsw-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
0 siblings, 1 reply; 27+ messages in thread
From: Scott Lovenberg @ 2013-07-19 19:32 UTC (permalink / raw)
To: Jeff Layton
Cc: Chen Gang, sfrench-eUNUBHrolfbYtjvyW6yDsg, linux-cifs,
samba-technical-w/Ol4Ecudpl8XjKLYN78aQ
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.
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH] cifs: extend the buffer length enought for sprintf() using
[not found] ` <CAFB9KM1cP1rvnO66+9kz+zVhVOf+C-dAR7mVMx_uX9hT-ORLsw-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
@ 2013-07-19 19:48 ` Jeff Layton
0 siblings, 0 replies; 27+ messages in thread
From: Jeff Layton @ 2013-07-19 19:48 UTC (permalink / raw)
To: Scott Lovenberg
Cc: Chen Gang, sfrench-eUNUBHrolfbYtjvyW6yDsg, linux-cifs,
samba-technical-w/Ol4Ecudpl8XjKLYN78aQ
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>
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2] cifs: extend the buffer length enought for sprintf() using
[not found] ` <20130719064531.2a9836f5-9yPaYZwiELC+kQycOl6kW4xkIHaj4LzF@public.gmane.org>
2013-07-19 15:46 ` Steve French
@ 2013-07-22 0:29 ` Chen Gang
1 sibling, 0 replies; 27+ messages in thread
From: Chen Gang @ 2013-07-22 0:29 UTC (permalink / raw)
To: Jeff Layton
Cc: Scott Lovenberg, Steve French, sfrench-eUNUBHrolfbYtjvyW6yDsg,
linux-cifs, samba-technical-w/Ol4Ecudpl8XjKLYN78aQ,
Shirish Pargaonkar
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
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2] cifs: extend the buffer length enought for sprintf() using
[not found] ` <CAH2r5mvccBQRikYbbUppmbCO1naSOOMJ+wVWxQEQBxhDdmnP_w-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2013-07-19 16:18 ` Steve French
@ 2013-07-22 1:21 ` Chen Gang
[not found] ` <51EC890D.7010306-bOixZGp5f+dBDgjK7y7TUQ@public.gmane.org>
1 sibling, 1 reply; 27+ messages in thread
From: Chen Gang @ 2013-07-22 1:21 UTC (permalink / raw)
To: Steve French
Cc: Jeff Layton, Scott Lovenberg, sfrench-eUNUBHrolfbYtjvyW6yDsg,
linux-cifs, samba-technical-w/Ol4Ecudpl8XjKLYN78aQ,
Shirish Pargaonkar
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
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2] cifs: extend the buffer length enought for sprintf() using
[not found] ` <51EC890D.7010306-bOixZGp5f+dBDgjK7y7TUQ@public.gmane.org>
@ 2013-07-31 4:59 ` Steve French
[not found] ` <CAH2r5mvPC0jiSDh5qTgerEV3JnRwcPPkKvnbT1QCO0CGiVngjg-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
0 siblings, 1 reply; 27+ messages in thread
From: Steve French @ 2013-07-31 4:59 UTC (permalink / raw)
To: Chen Gang
Cc: Jeff Layton, Scott Lovenberg, sfrench-eUNUBHrolfbYtjvyW6yDsg,
linux-cifs, samba-technical-w/Ol4Ecudpl8XjKLYN78aQ,
Shirish Pargaonkar
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
^ permalink raw reply related [flat|nested] 27+ messages in thread
* Re: [PATCH v2] cifs: extend the buffer length enought for sprintf() using
[not found] ` <CAH2r5mvPC0jiSDh5qTgerEV3JnRwcPPkKvnbT1QCO0CGiVngjg-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
@ 2013-07-31 15:55 ` Scott Lovenberg
0 siblings, 0 replies; 27+ messages in thread
From: Scott Lovenberg @ 2013-07-31 15:55 UTC (permalink / raw)
To: Steve French
Cc: Chen Gang, Jeff Layton, sfrench-eUNUBHrolfbYtjvyW6yDsg,
linux-cifs, samba-technical-w/Ol4Ecudpl8XjKLYN78aQ,
Shirish Pargaonkar
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.
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH] cifs: extend the buffer length enought for sprintf() using
[not found] ` <20130717212559.71b7af06-4QP7MXygkU+dMjc06nkz3ljfA9RmPOcC@public.gmane.org>
2013-07-18 1:31 ` Chen Gang
@ 2013-10-06 0:49 ` Chen Gang
1 sibling, 0 replies; 27+ messages in thread
From: Chen Gang @ 2013-10-06 0:49 UTC (permalink / raw)
To: Jeff Layton
Cc: Shirish Pargaonkar, Steve French, linux-cifs, samba-technical,
Richard Weinberger, Joe Perches, Al Viro, Thomas Gleixner
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
^ permalink raw reply [flat|nested] 27+ messages in thread
end of thread, other threads:[~2013-10-06 0:49 UTC | newest]
Thread overview: 27+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-07-17 0:48 [PATCH] cifs: extend the buffer length enought for sprintf() using Chen Gang
[not found] ` <51E5E9DA.8020603-bOixZGp5f+dBDgjK7y7TUQ@public.gmane.org>
2013-07-17 1:58 ` Scott Lovenberg
[not found] ` <CAFB9KM2nJEt-O+o=4bkxNMJ2Fr0TfjkpRF=7B98=Lp9zFu8_og-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2013-07-17 2:06 ` Steve French
[not found] ` <CAH2r5msQEbQWpE+wqEoLY_++=ywDVoAg_vmWB3kJG8=ECHC3Pg-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2013-07-17 2:11 ` Chen Gang
2013-07-17 18:27 ` Scott Lovenberg
[not found] ` <CAFB9KM0rEDyE6hb8t-gDLTDKq9kaRr4Bhs7SLBEZTnyH5u5U-A-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2013-07-18 1:08 ` Chen Gang
2013-07-18 6:47 ` Scott Lovenberg
[not found] ` <CAFB9KM2MbLuETpoN9wafZLq6B9StjtXnG15G16uGcOcnRM8+sA-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2013-07-19 1:01 ` [PATCH v2] " Chen Gang
[not found] ` <51E88FF0.2010101-bOixZGp5f+dBDgjK7y7TUQ@public.gmane.org>
2013-07-19 10:45 ` Jeff Layton
[not found] ` <20130719064531.2a9836f5-9yPaYZwiELC+kQycOl6kW4xkIHaj4LzF@public.gmane.org>
2013-07-19 15:46 ` Steve French
[not found] ` <CAH2r5mvccBQRikYbbUppmbCO1naSOOMJ+wVWxQEQBxhDdmnP_w-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2013-07-19 16:18 ` Steve French
2013-07-22 1:21 ` Chen Gang
[not found] ` <51EC890D.7010306-bOixZGp5f+dBDgjK7y7TUQ@public.gmane.org>
2013-07-31 4:59 ` Steve French
[not found] ` <CAH2r5mvPC0jiSDh5qTgerEV3JnRwcPPkKvnbT1QCO0CGiVngjg-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2013-07-31 15:55 ` Scott Lovenberg
2013-07-22 0:29 ` Chen Gang
2013-07-17 2:07 ` [PATCH] " Chen Gang
2013-07-19 17:51 ` Jeff Layton
[not found] ` <20130719135115.2ebff0cd-9yPaYZwiELC+kQycOl6kW4xkIHaj4LzF@public.gmane.org>
2013-07-19 19:32 ` Scott Lovenberg
[not found] ` <CAFB9KM1cP1rvnO66+9kz+zVhVOf+C-dAR7mVMx_uX9hT-ORLsw-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2013-07-19 19:48 ` Jeff Layton
2013-07-17 3:47 ` Shirish Pargaonkar
[not found] ` <CADT32e+Ydg5N8uWyvCKee8n4iS34LcCFgE1nrC2S8bq7GnA-hg-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2013-07-17 3:54 ` Steve French
2013-07-17 4:21 ` Chen Gang
2013-07-17 11:24 ` Jeff Layton
[not found] ` <20130717072431.5d8a22b3-9yPaYZwiELC+kQycOl6kW4xkIHaj4LzF@public.gmane.org>
2013-07-18 1:04 ` Chen Gang
[not found] ` <51E73F1E.4010804-bOixZGp5f+dBDgjK7y7TUQ@public.gmane.org>
2013-07-18 1:25 ` Jeff Layton
[not found] ` <20130717212559.71b7af06-4QP7MXygkU+dMjc06nkz3ljfA9RmPOcC@public.gmane.org>
2013-07-18 1:31 ` Chen Gang
2013-10-06 0:49 ` Chen Gang
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.