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

[Chen Gang]

On 07/17/2013 07:24 PM, Jeff Layton wrote:
> On Tue, 16 Jul 2013 22:47:35 -0500
> Shirish Pargaonkar <shirishpargaonkar at 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 at 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 at 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 at 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 at vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
> 


-- 
Chen Gang