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

[Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>]

On Thu, 18 Jul 2013 09:04:30 +0800
Chen Gang <gang.chen-bOixZGp5f+dBDgjK7y7TUQ@public.gmane.org> wrote:

> On 07/17/2013 07:24 PM, Jeff Layton wrote:
> > On Tue, 16 Jul 2013 22:47:35 -0500
> > Shirish Pargaonkar <shirishpargaonkar-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> > 
> >> nitpicking...
> >>
> >> Should it be MAX_CIFS_DOMAINNAME instead of MAX_CIF_DOMAINNAME,
> >> unless CIF is short for something here?
> >>
> >> Regards,
> >>
> >> Shirish
> >>
> > 
> > Even better...
> > 
> > We already have a MAX_USERNAME_SIZE. Maybe call it MAX_DOMAINNAME_SIZE
> > for parity with that? Might also want to relocate the #define next to
> > that one since it would be helpful to have all of those lengths grouped
> > in the same place.
> > 
> 
> It sounds reasonable: use MAX_DOMAINNAME_SIZE instead of
> MAX_CIFS_DOMAINNAME.
> 
> 
> >> On Tue, Jul 16, 2013 at 7:48 PM, Chen Gang <gang.chen-bOixZGp5f+dBDgjK7y7TUQ@public.gmane.org> wrote:
> >>> For cifs_set_cifscreds() in "fs/cifs/connect.c", 'desc' buffer length
> >>> is 'CIFSCREDS_DESC_SIZE' (56 is less than 256), and 'ses->domainName'
> >>> length may be "255 + '\0'".
> >>>
> >>> The related sprintf() may cause memory overflow, so need extend related
> >>> buffer enough to hold all things.
> >>>
> > 
> > Good catch...
> > 
> > Maybe it would be good to go ahead and turn that sprintf() into a
> > snprintf() too?
> > 
> 
> Hmm... sprintf() declares to code readers, in current condition, we want
> to print all source information without any truncation.
> 
> So if we know the source max length precisely, we'd better to allocate
> the related buffer to print them all instead of use snprintf().
> 
> If we do not know the source max length precisely or we have to limit
> the destination length, we need use snprintf() to fit with destination
> max length (declare to the code readers, the source information may be
> truncated).
> 
> 

Fair enough. It was more of a suggestion for defensive coding. But
since we know the length of both buffers and that the source is NULL
terminated, then sprintf is fine.

Patch looks fine to me otherwise.

Acked-by: Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>