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

[Chen Gang]

On 07/18/2013 09:25 AM, Jeff Layton wrote:
> On Thu, 18 Jul 2013 09:04:30 +0800
> Chen Gang <gang.chen at 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 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).
>> > 
>> > 
> 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 at 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.
-- 
Chen Gang