cifs: extend the buffer length enought for sprintf() using
Subsystems (2)
PATCH 1/1 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'.
--- 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(-)
fs/cifs/cifsencrypt.c +1 -1
@@ -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 =
fs/cifs/cifsglob.h +1 -0
@@ -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;
fs/cifs/connect.c +4 -3
@@ -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
fs/cifs/sess.c +3 -3
@@ -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 ;
Signature · 1 line
1.7.7.6
A message that is not in the archive (sent off-list or not yet received): [email protected]
Quoted 93 lines · Chen Gang
For
cifs_set_cifscreds()in "fs/cifs/connect.c", 'desc' buffer length is 'CIFSCREDS_DESC_SIZE' (56 is less than 256), and 'ses->domainName' length may be "255 + '\0'".The related
sprintf()may cause memory overflow, so need extend related buffer enough to hold all things.It is also necessary to be sure of '
ses->domainName' must be less than 256, and define the related macro instead of hard code number '256'.Signed-off-by Chen Gang <[email protected]>--- 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(-)fs/cifs/cifsencrypt.c +1
-1@@ -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 =fs/cifs/cifsglob.h +1
-0@@ -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;fs/cifs/connect.c +4
-3@@ -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 intfs/cifs/sess.c +3
-4@@ -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 ; --
Quoted 5 lines
1.7.7.6
--
To unsubscribe from this list: send the line "unsubscribe linux-cifs" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
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.
A message that is not in the archive (sent off-list or not yet received): CAFB9KM2nJEt-O+o=4bkxNMJ2Fr0TfjkpRF=7B98=Lp9zFu8_og-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org
Quoted 105 lines · Scott Lovenberg
Quoted 93 lines · Chen Gang
For
cifs_set_cifscreds()in "fs/cifs/connect.c", 'desc' buffer length is 'CIFSCREDS_DESC_SIZE' (56 is less than 256), and 'ses->domainName' length may be "255 + '\0'".The related
sprintf()may cause memory overflow, so need extend related buffer enough to hold all things.It is also necessary to be sure of '
ses->domainName' must be less than 256, and define the related macro instead of hard code number '256'.Signed-off-by Chen Gang <[email protected]>--- 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(-)fs/cifs/cifsencrypt.c +1
-1@@ -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 =fs/cifs/cifsglob.h +1
-0@@ -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;fs/cifs/connect.c +4
-3@@ -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 intfs/cifs/sess.c +3
-4@@ -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 ; --Quoted 5 lines
1.7.7.6
--
To unsubscribe from this list: send the line "unsubscribe linux-cifs" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.htmlIf this is correct for the domain size,
MAX_DOMAIN_SIZEin 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.
Quoted 5 lines
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."
Signature · 3 lines
Thanks, Steve
A message that is not in the archive (sent off-list or not yet received): CAH2r5msQEbQWpE+wqEoLY_++=ywDVoAg_vmWB3kJG8=ECHC3Pg-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org
Quoted 118 lines · Steve French
Quoted 105 lines · Scott Lovenberg
Quoted 105 lines · Chen Gang
Quoted 83 lines
Quoted 83 lines
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 <[email protected]>--- 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(-)fs/cifs/cifsencrypt.c +1
-1@@ -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 =fs/cifs/cifsglob.h +1
-0@@ -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;fs/cifs/connect.c +4
-3@@ -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 intfs/cifs/sess.c +1
-1@@ -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,Quoted 15 lines
Quoted 10 lines
fs/cifs/sess.c +2
-3/* 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 ; --Quoted 5 lines
1.7.7.6
--
To unsubscribe from this list: send the line "unsubscribe linux-cifs" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.htmlIf this is correct for the domain size,
MAX_DOMAIN_SIZEin 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.Quoted 5 lines
Quoted 5 lines
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.
Quoted 3 lines
"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.
Signature · 1 line
Chen Gang
Quoted 22 lines · Steve French
Quoted 10 lines
If this is correct for the domain size,
MAX_DOMAIN_SIZEin 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."
Quoted 4 lines
--
Thanks,Steve
Thanks, Steve. I'll patch this tonight.
--
Peace and Blessings,
-Scott.
A message that is not in the archive (sent off-list or not yet received): CAFB9KM0rEDyE6hb8t-gDLTDKq9kaRr4Bhs7SLBEZTnyH5u5U-A-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org
Quoted 24 lines · Scott Lovenberg
Quoted 22 lines · Steve French
Quoted 10 lines
If this is correct for the domain size,
MAX_DOMAIN_SIZEin 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."
Quoted 7 lines
Quoted 4 lines
--
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 ?
Quoted 6 lines
--
Peace and Blessings,
-Scott.
Thanks.
Signature · 1 line
Chen Gang
Quoted 38 lines · Chen Gang
Quoted 24 lines · Scott Lovenberg
Quoted 22 lines · Steve French
Quoted 10 lines
If this is correct for the domain size,
MAX_DOMAIN_SIZEin 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."
Quoted 7 lines
Quoted 4 lines
--
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 ?
Quoted 8 lines
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.
A message that is not in the archive (sent off-list or not yet received): CAFB9KM2MbLuETpoN9wafZLq6B9StjtXnG15G16uGcOcnRM8+sA-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org
PATCH v2 1/1 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'.
--- 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(-)
fs/cifs/cifsencrypt.c +1 -1
@@ -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 =
fs/cifs/cifsglob.h +1 -0
@@ -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 */
fs/cifs/connect.c +4 -3
@@ -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
fs/cifs/sess.c +3 -3
@@ -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 ;
Signature · 1 line
1.7.7.6
A message that is not in the archive (sent off-list or not yet received): [email protected]
Jeff Layton
Reviewed-byQuoted 82 lines · On Fri, 19 Jul 2013 09:01:36 +0800 Chen Gang…
For
cifs_set_cifscreds()in "fs/cifs/connect.c", 'desc' buffer length is 'CIFSCREDS_DESC_SIZE' (56 is less than 256), and 'ses->domainName' length may be "255 + '\0'".The related
sprintf()may cause memory overflow, so need extend related buffer enough to hold all things.It is also necessary to be sure of '
ses->domainName' must be less than 256, and define the related macro instead of hard code number '256'.Signed-off-by Chen Gang <[email protected]>--- 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(-)fs/cifs/cifsencrypt.c +1
-1@@ -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 =fs/cifs/cifsglob.h +1
-0@@ -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 */ fs/cifs/connect.c +4
-3@@ -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 intfs/cifs/sess.c +1
-1@@ -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 */
fs/cifs/sess.c static void ascii_ssetup_strings(char **pbcc_area, struct cifs_ses *ses, · line 262
Quoted 10 lines
fs/cifs/sess.c +2
-2@@ -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...
A message that is not in the archive (sent off-list or not yet received): 20130719064531.2a9836f5-9yPaYZwiELC+kQycOl6kW4xkIHaj4LzF@public.gmane.org
Steve French
Appliedmerged into cifs-2.6.git
Quoted 99 lines · Jeff Layton
Quoted 82 lines · On Fri, 19 Jul 2013 09:01:36 +0800 Chen Gang…
For
cifs_set_cifscreds()in "fs/cifs/connect.c", 'desc' buffer length is 'CIFSCREDS_DESC_SIZE' (56 is less than 256), and 'ses->domainName' length may be "255 + '\0'".The related
sprintf()may cause memory overflow, so need extend related buffer enough to hold all things.It is also necessary to be sure of '
ses->domainName' must be less than 256, and define the related macro instead of hard code number '256'.Signed-off-by Chen Gang <[email protected]>--- 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(-)fs/cifs/cifsencrypt.c +1
-1@@ -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 =fs/cifs/cifsglob.h +1
-0@@ -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 */ fs/cifs/connect.c +4
-3@@ -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 intfs/cifs/sess.c +1
-1@@ -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 */ fs/cifs/sess.c static void ascii_ssetup_strings(char **pbcc_area, struct cifs_ses *ses, · line 262
Quoted 10 lines
fs/cifs/sess.c +2
-2@@ -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 <[email protected]>
Signature · 3 lines
Thanks, Steve
A message that is not in the archive (sent off-list or not yet received): CAH2r5mvccBQRikYbbUppmbCO1naSOOMJ+wVWxQEQBxhDdmnP_w-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org
Added CC: <[email protected]>
Quoted 109 lines · Steve French
merged into cifs-2.6.git
Quoted 98 lines · Jeff Layton
Quoted 82 lines · On Fri, 19 Jul 2013 09:01:36 +0800 Chen Gang…
For
cifs_set_cifscreds()in "fs/cifs/connect.c", 'desc' buffer length is 'CIFSCREDS_DESC_SIZE' (56 is less than 256), and 'ses->domainName' length may be "255 + '\0'".The related
sprintf()may cause memory overflow, so need extend related buffer enough to hold all things.It is also necessary to be sure of '
ses->domainName' must be less than 256, and define the related macro instead of hard code number '256'.Signed-off-by Chen Gang <[email protected]>--- 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(-)fs/cifs/cifsencrypt.c +1
-1@@ -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 =fs/cifs/cifsglob.h +1
-0@@ -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 */ fs/cifs/connect.c +4
-3@@ -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 intfs/cifs/sess.c +1
-1@@ -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 */ fs/cifs/sess.c static void ascii_ssetup_strings(char **pbcc_area, struct cifs_ses *ses, · line 262
Quoted 10 lines
fs/cifs/sess.c +2
-2@@ -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...
Quoted 1 line
Reviewed-by Jeff Layton <[email protected]>--
Thanks,Steve
Signature · 3 lines
Thanks, Steve
Quoted 2 lines · Steve French
merged into cifs-2.6.git
Thanks.
Quoted 104 lines · Jeff Layton
Quoted 98 lines
Quoted 98 lines
Quoted 83 lines · On Fri, 19 Jul 2013 09:01:36 +0800 Chen Gang…
Quoted 83 lines
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 <[email protected]>--- 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(-)fs/cifs/cifsencrypt.c +1
-1@@ -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 =fs/cifs/cifsglob.h +1
-0@@ -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 */ fs/cifs/connect.c +4
-3@@ -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 intfs/cifs/sess.c +1
-1@@ -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,Quoted 9 lines
fs/cifs/sess.c static void ascii_ssetup_strings(char **pbcc_area, struct cifs_ses *ses, · line 262
Quoted 9 lines
fs/cifs/sess.c +2
-2/* 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...
Quoted 1 line
Quoted 1 line
Reviewed-by Jeff Layton <[email protected]>-- Thanks, Steve
Signature · 1 line
Chen Gang
A message that is not in the archive (sent off-list or not yet received): [email protected]
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 <[email protected]> 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'.
fs/cifs/cifsencrypt.c +0 -0
@@ -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 =
fs/cifs/cifsglob.h +1 -0
@@ -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 */
fs/cifs/connect.c +0 -0
@@ -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
fs/cifs/sess.c +0 -0
@@ -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;
Quoted 114 lines · Chen Gang
Quoted 2 lines · Steve French
merged into cifs-2.6.git
Thanks.
Quoted 99 lines · Jeff Layton
Quoted 98 lines
Quoted 98 lines
Quoted 83 lines · On Fri, 19 Jul 2013 09:01:36 +0800 Chen Gang…
Quoted 83 lines
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 <[email protected]>--- 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(-)fs/cifs/cifsencrypt.c +1
-1@@ -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 =fs/cifs/cifsglob.h +1
-0@@ -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 */ fs/cifs/connect.c +4
-3@@ -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 intfs/cifs/sess.c +1
-1@@ -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,Quoted 9 lines
fs/cifs/sess.c static void ascii_ssetup_strings(char **pbcc_area, struct cifs_ses *ses, · line 262
Quoted 9 lines
fs/cifs/sess.c +2
-2/* 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...
Quoted 5 lines
Quoted 1 line
Quoted 1 line
Reviewed-by Jeff Layton <[email protected]>-- Thanks, Steve
--
Chen Gang
Signature · 3 lines
Thanks, Steve
A message that is not in the archive (sent off-list or not yet received): CAH2r5mvPC0jiSDh5qTgerEV3JnRwcPPkKvnbT1QCO0CGiVngjg-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org
Quoted 9 lines · Steve French
I updated the version of the patch slightly which is in cifs-2.6.git so we don't have to rename the
MAX_DOMAINNAME_SIZEfield 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.
Signature · 2 lines
Peace and Blessings, -Scott.
Quoted 98 lines · Jeff Layton
Quoted 82 lines · On Fri, 19 Jul 2013 09:01:36 +0800 Chen Gang…
For
cifs_set_cifscreds()in "fs/cifs/connect.c", 'desc' buffer length is 'CIFSCREDS_DESC_SIZE' (56 is less than 256), and 'ses->domainName' length may be "255 + '\0'".The related
sprintf()may cause memory overflow, so need extend related buffer enough to hold all things.It is also necessary to be sure of '
ses->domainName' must be less than 256, and define the related macro instead of hard code number '256'.Signed-off-by Chen Gang <[email protected]>--- 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(-)fs/cifs/cifsencrypt.c +1
-1@@ -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 =fs/cifs/cifsglob.h +1
-0@@ -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 */ fs/cifs/connect.c +4
-3@@ -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 intfs/cifs/sess.c +1
-1@@ -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 */ fs/cifs/sess.c static void ascii_ssetup_strings(char **pbcc_area, struct cifs_ses *ses, · line 262
Quoted 10 lines
fs/cifs/sess.c +2
-2@@ -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...
Quoted 3 lines
Reviewed-by Jeff Layton <[email protected]>
Thanks.
Signature · 1 line
Chen Gang
Quoted 104 lines · Scott Lovenberg
Quoted 83 lines · Chen Gang
Quoted 83 lines
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 <[email protected]>--- 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(-)fs/cifs/cifsencrypt.c +1
-1@@ -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 =fs/cifs/cifsglob.h +1
-0@@ -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;fs/cifs/connect.c +4
-3@@ -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 intfs/cifs/sess.c +1
-1@@ -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,Quoted 15 lines
Quoted 10 lines
fs/cifs/sess.c +2
-3/* 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 ; --Quoted 5 lines
1.7.7.6
--
To unsubscribe from this list: send the line "unsubscribe linux-cifs" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.htmlIf this is correct for the domain size,
MAX_DOMAIN_SIZEin 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.
Quoted 1 line
CC'ing Jeff Layton since he maintains the cifs-utils package.
Thank you very much.
Signature · 1 line
Chen Gang
Quoted 105 lines · On Tue, 16 Jul 2013 21:58:19 -0400 Scott Lovenberg…
Quoted 93 lines · Chen Gang
For
cifs_set_cifscreds()in "fs/cifs/connect.c", 'desc' buffer length is 'CIFSCREDS_DESC_SIZE' (56 is less than 256), and 'ses->domainName' length may be "255 + '\0'".The related
sprintf()may cause memory overflow, so need extend related buffer enough to hold all things.It is also necessary to be sure of '
ses->domainName' must be less than 256, and define the related macro instead of hard code number '256'.Signed-off-by Chen Gang <[email protected]>--- 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(-)fs/cifs/cifsencrypt.c +1
-1@@ -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 =fs/cifs/cifsglob.h +1
-0@@ -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;fs/cifs/connect.c +4
-3@@ -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 intfs/cifs/sess.c +3
-4@@ -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 ; --Quoted 5 lines
1.7.7.6
--
To unsubscribe from this list: send the line "unsubscribe linux-cifs" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.htmlIf this is correct for the domain size,
MAX_DOMAIN_SIZEin 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.
Quoted 2 lines
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.
Signature · 1 line
Jeff Layton <[email protected]>
A message that is not in the archive (sent off-list or not yet received): 20130719135115.2ebff0cd-9yPaYZwiELC+kQycOl6kW4xkIHaj4LzF@public.gmane.org
Quoted 109 lines · Jeff Layton
Quoted 105 lines · On Tue, 16 Jul 2013 21:58:19 -0400 Scott Lovenberg…
Quoted 93 lines · Chen Gang
For
cifs_set_cifscreds()in "fs/cifs/connect.c", 'desc' buffer length is 'CIFSCREDS_DESC_SIZE' (56 is less than 256), and 'ses->domainName' length may be "255 + '\0'".The related
sprintf()may cause memory overflow, so need extend related buffer enough to hold all things.It is also necessary to be sure of '
ses->domainName' must be less than 256, and define the related macro instead of hard code number '256'.Signed-off-by Chen Gang <[email protected]>--- 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(-)fs/cifs/cifsencrypt.c +1
-1@@ -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 =fs/cifs/cifsglob.h +1
-0@@ -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;fs/cifs/connect.c +4
-3@@ -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 intfs/cifs/sess.c +3
-4@@ -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 ; --Quoted 5 lines
1.7.7.6
--
To unsubscribe from this list: send the line "unsubscribe linux-cifs" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.htmlIf this is correct for the domain size,
MAX_DOMAIN_SIZEin 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.
Quoted 8 lines
Quoted 2 lines
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 <[email protected]>
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.
A message that is not in the archive (sent off-list or not yet received): CAFB9KM1cP1rvnO66+9kz+zVhVOf+C-dAR7mVMx_uX9hT-ORLsw-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org
Quoted 130 lines · On Fri, 19 Jul 2013 15:32:29 -0400 Scott Lovenberg…
Quoted 109 lines · Jeff Layton
Quoted 105 lines · On Tue, 16 Jul 2013 21:58:19 -0400 Scott Lovenberg…
Quoted 93 lines · Chen Gang
For
cifs_set_cifscreds()in "fs/cifs/connect.c", 'desc' buffer length is 'CIFSCREDS_DESC_SIZE' (56 is less than 256), and 'ses->domainName' length may be "255 + '\0'".The related
sprintf()may cause memory overflow, so need extend related buffer enough to hold all things.It is also necessary to be sure of '
ses->domainName' must be less than 256, and define the related macro instead of hard code number '256'.Signed-off-by Chen Gang <[email protected]>--- 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(-)fs/cifs/cifsencrypt.c +1
-1@@ -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 =fs/cifs/cifsglob.h +1
-0@@ -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;fs/cifs/connect.c +4
-3@@ -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 intfs/cifs/sess.c +3
-4@@ -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 ; --Quoted 5 lines
1.7.7.6
--
To unsubscribe from this list: send the line "unsubscribe linux-cifs" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.htmlIf this is correct for the domain size,
MAX_DOMAIN_SIZEin 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.Quoted 8 lines
Quoted 2 lines
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 <[email protected]>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.
Quoted 3 lines
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...
Signature · 1 line
Jeff Layton <[email protected]>
nitpicking...
Should it be MAX_CIFS_DOMAINNAME instead of MAX_CIF_DOMAINNAME, unless CIF is short for something here?
Regards,
Shirish
Quoted 98 lines · Chen Gang
For
cifs_set_cifscreds()in "fs/cifs/connect.c", 'desc' buffer length is 'CIFSCREDS_DESC_SIZE' (56 is less than 256), and 'ses->domainName' length may be "255 + '\0'".The related
sprintf()may cause memory overflow, so need extend related buffer enough to hold all things.It is also necessary to be sure of '
ses->domainName' must be less than 256, and define the related macro instead of hard code number '256'.Signed-off-by Chen Gang <[email protected]>--- 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(-)fs/cifs/cifsencrypt.c +1
-1@@ -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 =fs/cifs/cifsglob.h +1
-0@@ -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;fs/cifs/connect.c +4
-3@@ -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 intfs/cifs/sess.c +3
-4@@ -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 [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
A message that is not in the archive (sent off-list or not yet received): CADT32e+Ydg5N8uWyvCKee8n4iS34LcCFgE1nrC2S8bq7GnA-hg-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org
I assumed it was an 80 column thing - but don't know.
CIFS instead of CIF would be the normal
Quoted 108 lines · Shirish Pargaonkar
nitpicking...
Should it be
MAX_CIFS_DOMAINNAMEinstead ofMAX_CIF_DOMAINNAME, unless CIF is short for something here?Regards,
Shirish
Quoted 98 lines · Chen Gang
For
cifs_set_cifscreds()in "fs/cifs/connect.c", 'desc' buffer length is 'CIFSCREDS_DESC_SIZE' (56 is less than 256), and 'ses->domainName' length may be "255 + '\0'".The related
sprintf()may cause memory overflow, so need extend related buffer enough to hold all things.It is also necessary to be sure of '
ses->domainName' must be less than 256, and define the related macro instead of hard code number '256'.Signed-off-by Chen Gang <[email protected]>--- 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(-)fs/cifs/cifsencrypt.c +1
-1@@ -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 =fs/cifs/cifsglob.h +1
-0@@ -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;fs/cifs/connect.c +4
-3@@ -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 intfs/cifs/sess.c +3
-4@@ -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 [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
Signature · 3 lines
Thanks, Steve
Quoted 5 lines · Shirish Pargaonkar
nitpicking...
Should it be
MAX_CIFS_DOMAINNAMEinstead ofMAX_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.
Quoted 105 lines
Regards,
Shirish
Quoted 98 lines · Chen Gang
For
cifs_set_cifscreds()in "fs/cifs/connect.c", 'desc' buffer length is 'CIFSCREDS_DESC_SIZE' (56 is less than 256), and 'ses->domainName' length may be "255 + '\0'".The related
sprintf()may cause memory overflow, so need extend related buffer enough to hold all things.It is also necessary to be sure of '
ses->domainName' must be less than 256, and define the related macro instead of hard code number '256'.Signed-off-by Chen Gang <[email protected]>--- 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(-)fs/cifs/cifsencrypt.c +1
-1@@ -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 =fs/cifs/cifsglob.h +1
-0@@ -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;fs/cifs/connect.c +4
-3@@ -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 intfs/cifs/sess.c +3
-4@@ -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 [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
Signature · 1 line
Chen Gang
Quoted 9 lines · On Tue, 16 Jul 2013 22:47:35 -0500 Shirish Pargaonkar…
nitpicking...
Should it be
MAX_CIFS_DOMAINNAMEinstead ofMAX_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.
Quoted 8 lines · Chen Gang
Quoted 7 lines
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?
Quoted 95 lines
Quoted 86 lines
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 <[email protected]>--- 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(-)fs/cifs/cifsencrypt.c +1
-1@@ -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 =fs/cifs/cifsglob.h +1
-0@@ -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;fs/cifs/connect.c +4
-3@@ -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 intfs/cifs/sess.c +3
-4@@ -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 ; --Quoted 5 lines
1.7.7.6
--
To unsubscribe from this list: send the line "unsubscribe linux-cifs" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html--
To unsubscribe from this list: send the line "unsubscribe linux-cifs" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
Signature · 1 line
Jeff Layton <[email protected]>
A message that is not in the archive (sent off-list or not yet received): 20130717072431.5d8a22b3-9yPaYZwiELC+kQycOl6kW4xkIHaj4LzF@public.gmane.org
Quoted 13 lines · Jeff Layton
Quoted 9 lines · On Tue, 16 Jul 2013 22:47:35 -0500 Shirish Pargaonkar…
nitpicking...
Should it be
MAX_CIFS_DOMAINNAMEinstead ofMAX_CIF_DOMAINNAME, unless CIF is short for something here?Regards,
Shirish
Quoted 7 lines
Even better...
We already have a
MAX_USERNAME_SIZE. Maybe call itMAX_DOMAINNAME_SIZEfor 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.
Quoted 5 lines
Quoted 5 lines · Chen Gang
Quoted 4 lines
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'".
Quoted 9 lines
Quoted 3 lines
Quoted 3 lines
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 asnprintf()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.
Quoted 97 lines
Quoted 95 lines
Quoted 86 lines
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 <[email protected]>--- 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(-)fs/cifs/cifsencrypt.c +1
-1@@ -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 =fs/cifs/cifsglob.h +1
-0@@ -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;fs/cifs/connect.c +4
-3@@ -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 intfs/cifs/sess.c +3
-4@@ -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 ; --Quoted 5 lines
1.7.7.6
--
To unsubscribe from this list: send the line "unsubscribe linux-cifs" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html--
To unsubscribe from this list: send the line "unsubscribe linux-cifs" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
Signature · 1 line
Chen Gang
A message that is not in the archive (sent off-list or not yet received): [email protected]
Jeff Layton
Acked-byQuoted 44 lines · On Thu, 18 Jul 2013 09:04:30 +0800 Chen Gang…
Quoted 13 lines · Jeff Layton
Quoted 9 lines · On Tue, 16 Jul 2013 22:47:35 -0500 Shirish Pargaonkar…
nitpicking...
Should it be
MAX_CIFS_DOMAINNAMEinstead ofMAX_CIF_DOMAINNAME, unless CIF is short for something here?Regards,
Shirish
Quoted 7 lines
Even better...
We already have a
MAX_USERNAME_SIZE. Maybe call itMAX_DOMAINNAME_SIZEfor 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_SIZEinstead ofMAX_CIFS_DOMAINNAME.Quoted 5 lines
Quoted 5 lines · Chen Gang
Quoted 4 lines
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'".Quoted 9 lines
Quoted 3 lines
Quoted 3 lines
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 asnprintf()too?Hmm...
sprintf()declares to code readers, in current condition, we want to print all source information without any truncation.
Quoted 9 lines
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.
A message that is not in the archive (sent off-list or not yet received): 20130717212559.71b7af06-4QP7MXygkU+dMjc06nkz3ljfA9RmPOcC@public.gmane.org
Quoted 56 lines · Jeff Layton
Quoted 47 lines · On Thu, 18 Jul 2013 09:04:30 +0800 Chen Gang…
Quoted 47 lines · Jeff Layton
Quoted 13 lines
Quoted 13 lines
Quoted 9 lines · On Tue, 16 Jul 2013 22:47:35 -0500 Shirish Pargaonkar…
Quoted 9 lines
nitpicking...
Should it be
MAX_CIFS_DOMAINNAMEinstead ofMAX_CIF_DOMAINNAME, unless CIF is short for something here?Regards,
Shirish
Quoted 7 lines
Quoted 7 lines
Even better...
We already have a
MAX_USERNAME_SIZE. Maybe call itMAX_DOMAINNAME_SIZEfor 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_SIZEinstead ofMAX_CIFS_DOMAINNAME.Quoted 9 lines
Quoted 9 lines
Quoted 8 lines
Quoted 8 lines · Chen Gang
Quoted 7 lines
Quoted 7 lines
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.Quoted 5 lines
Quoted 5 lines
Good catch...
Maybe it would be good to go ahead and turn that
sprintf()into asnprintf()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().Quoted 6 lines
Quoted 6 lines
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).
Quoted 9 lines
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 <[email protected]>
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.
Signature · 1 line
Chen Gang
Quoted 10 lines · Jeff Layton
Quoted 10 lines
Quoted 3 lines
Quoted 3 lines
Maybe it would be good to go ahead and turn that
sprintf()into asnprintf()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().
Quoted 6 lines
Quoted 6 lines
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.
Signature · 1 line
Chen Gang