| Message ID | 51E5E9DA.8020603@asianux.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Jul 16, 2013 at 8:48 PM, Chen Gang <gang.chen@asianux.com> 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@asianux.com> > --- > 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@vger.kernel.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. -- To unsubscribe from this list: send the line "unsubscribe linux-cifs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Jul 16, 2013 at 8:58 PM, Scott Lovenberg <scott.lovenberg@gmail.com> wrote: > On Tue, Jul 16, 2013 at 8:48 PM, Chen Gang <gang.chen@asianux.com> 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@asianux.com> >> --- >> 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@vger.kernel.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."
On 07/17/2013 09:58 AM, Scott Lovenberg wrote: > On Tue, Jul 16, 2013 at 8:48 PM, Chen Gang <gang.chen@asianux.com> 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@asianux.com> >> > --- >> > 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@vger.kernel.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.
On 07/17/2013 10:06 AM, Steve French wrote: > On Tue, Jul 16, 2013 at 8:58 PM, Scott Lovenberg > <scott.lovenberg@gmail.com> wrote: >> > On Tue, Jul 16, 2013 at 8:48 PM, Chen Gang <gang.chen@asianux.com> 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@asianux.com> >>> >> --- >>> >> 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@vger.kernel.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.
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@asianux.com> 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@asianux.com> > --- > 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@vger.kernel.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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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@gmail.com> 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@asianux.com> 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@asianux.com> >> --- >> 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@vger.kernel.org >> More majordomo info at http://vger.kernel.org/majordomo-info.html
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@asianux.com> 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@asianux.com> >> --- >> 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@vger.kernel.org >> More majordomo info at http://vger.kernel.org/majordomo-info.html > >
On Tue, 16 Jul 2013 22:47:35 -0500 Shirish Pargaonkar <shirishpargaonkar@gmail.com> 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@asianux.com> 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@asianux.com> > > --- > > 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@vger.kernel.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@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html
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. -- Peace and Blessings, -Scott. -- To unsubscribe from this list: send the line "unsubscribe linux-cifs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 07/17/2013 07:24 PM, Jeff Layton wrote: > On Tue, 16 Jul 2013 22:47:35 -0500 > Shirish Pargaonkar <shirishpargaonkar@gmail.com> 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@asianux.com> 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@asianux.com> >>> --- >>> 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@vger.kernel.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@vger.kernel.org >> More majordomo info at http://vger.kernel.org/majordomo-info.html > >
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 ? > > -- > Peace and Blessings, > -Scott. > > Thanks.
On Thu, 18 Jul 2013 09:04:30 +0800 Chen Gang <gang.chen@asianux.com> wrote: > On 07/17/2013 07:24 PM, Jeff Layton wrote: > > On Tue, 16 Jul 2013 22:47:35 -0500 > > Shirish Pargaonkar <shirishpargaonkar@gmail.com> 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@asianux.com> 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@redhat.com> -- To unsubscribe from this list: send the line "unsubscribe linux-cifs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 07/18/2013 09:25 AM, Jeff Layton wrote: > On Thu, 18 Jul 2013 09:04:30 +0800 > Chen Gang <gang.chen@asianux.com> wrote: > >> > On 07/17/2013 07:24 PM, Jeff Layton wrote: >>> > > On Tue, 16 Jul 2013 22:47:35 -0500 >>> > > Shirish Pargaonkar <shirishpargaonkar@gmail.com> 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@asianux.com> 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@redhat.com> > > 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.
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. -- To unsubscribe from this list: send the line "unsubscribe linux-cifs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, 16 Jul 2013 21:58:19 -0400 Scott Lovenberg <scott.lovenberg@gmail.com> wrote: > On Tue, Jul 16, 2013 at 8:48 PM, Chen Gang <gang.chen@asianux.com> 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@asianux.com> > > --- > > 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@vger.kernel.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.
On Fri, Jul 19, 2013 at 1:51 PM, Jeff Layton <jlayton@redhat.com> wrote: > > On Tue, 16 Jul 2013 21:58:19 -0400 > Scott Lovenberg <scott.lovenberg@gmail.com> wrote: > > > On Tue, Jul 16, 2013 at 8:48 PM, Chen Gang <gang.chen@asianux.com> 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@asianux.com> > > > --- > > > 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@vger.kernel.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@redhat.com> 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. -- To unsubscribe from this list: send the line "unsubscribe linux-cifs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, 19 Jul 2013 15:32:29 -0400 Scott Lovenberg <scott.lovenberg@gmail.com> wrote: > On Fri, Jul 19, 2013 at 1:51 PM, Jeff Layton <jlayton@redhat.com> wrote: > > > > On Tue, 16 Jul 2013 21:58:19 -0400 > > Scott Lovenberg <scott.lovenberg@gmail.com> wrote: > > > > > On Tue, Jul 16, 2013 at 8:48 PM, Chen Gang <gang.chen@asianux.com> 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@asianux.com> > > > > --- > > > > 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@vger.kernel.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@redhat.com> > > > 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...
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.
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;
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@asianux.com> --- 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(-)