Re: [PATCH 2/2] cifs: Increase size of tmp_buf in cifs_readdir to avoid potential overflows

On Tue, 21 Apr 2009 04:24:43 +0200
Günter Kukkukk <[email protected]> wrote:

> Am Montag, 20. April 2009 schrieb Steve French:
> > Merged this and also patch 1 of 2
> > 
> > thx
> > 
> > On Mon, Apr 20, 2009 at 10:30 AM, Jeff Layton <[email protected]> wrote:
> > > On Mon, 20 Apr 2009 18:54:36 +0530
> > > Suresh Jayaraman <[email protected]> wrote:
> > >
> > >> Increase size of tmp_buf to possible maximum to avoid potential
> > >> overflows.
> > >>
> > >>
> > >> Pointed-out-by: Jeff Layton <[email protected]>
> > >> Signed-off-by: Suresh Jayaraman <[email protected]>
> > >> ---
> > >>  fs/cifs/readdir.c |    2 +-
> > >>  1 files changed, 1 insertions(+), 1 deletions(-)
> > >>
> > >> diff --git a/fs/cifs/readdir.c b/fs/cifs/readdir.c
> > >> index 1a8be62..ebd0da7 100644
> > >> --- a/fs/cifs/readdir.c
> > >> +++ b/fs/cifs/readdir.c
> > >> @@ -1074,7 +1074,7 @@ int cifs_readdir(struct file *file, void *direntry, filldir_t filldir)
> > >>               with the rare long characters alloc more to account for
> > >>               such multibyte target UTF-8 characters. cifs_unicode.c,
> > >>               which actually does the conversion, has the same limit */
> > >> -             tmp_buf = kmalloc((2 * NAME_MAX) + 4, GFP_KERNEL);
> > >> +             tmp_buf = kmalloc((4 * NAME_MAX) + 2, GFP_KERNEL);
> > >>               for (i = 0; (i < num_to_fill) && (rc == 0); i++) {
> > >>                       if (current_entry == NULL) {
> > >>                               /* evaluate whether this case is an error */
> > >
> > > Acked-by: Jeff Layton <[email protected]>
> > >
> > 
> > 
> > 
> 
> I think patch 1 
>   - cifs: Rename cifs_strncpy_to_host and fix buffer size
> is wrong (which also results from using weak variables names).
> 
>                (*dst)[plen] = 0;
>                (*dst)[plen+1] = 0; /* needed for Unicode */
> 
> is using _source_ "plen" length when applied to dest. string.
> In addition, hardcoded (numbered) stuff like this
>    *dst = kmalloc((4 * plen) + 2, GFP_KERNEL);
> is also wrong.
> It's based on todays (right) assumption, that an UTF-8 string
> can only consume max. 4 bytes per char.
> 
> But - the maximum length is defined in:
> ./include/linux/nls.h:#define NLS_MAX_CHARSET_SIZE 6 /* for UTF-8 */
> 
> At least that hardcoded "4" should be carefully defined somewhere - if
> ever used!
> 
> my current diff (not tested) - temporary:
> diff --git a/fs/cifs/cifssmb.c b/fs/cifs/cifssmb.c
> index a02c43b..c9dcc1b 100644
> --- a/fs/cifs/cifssmb.c
> +++ b/fs/cifs/cifssmb.c
> @@ -91,22 +91,25 @@ static int
>  cifs_strlcpy_to_host(char **dst, const char *src, const int maxlen,
>                  const bool is_unicode, const struct nls_table *nls_codepage)
>  {
> -       int plen;
> +       int src_len, dst_len;
> 
>         if (is_unicode) {
> -               plen = UniStrnlen((wchar_t *)src, maxlen);
> -               *dst = kmalloc((4 * plen) + 2, GFP_KERNEL);
> +               src_len = UniStrnlen((wchar_t *)src, maxlen);
> +               *dst = kmalloc((NLS_MAX_CHARSET_SIZE * src_len) + 2, GFP_KERNEL);
>                 if (!*dst)
>                         goto cifs_strlcpy_to_host_ErrExit;
> -               cifs_strfromUCS_le(*dst, (__le16 *)src, plen, nls_codepage);
> -               (*dst)[plen] = 0;
> -               (*dst)[plen+1] = 0; /* needed for Unicode */
> +               dst_len = cifs_strfromUCS_le(*dst, (__le16 *)src, plen, nls_codepage);
> +               /* Note:
> +                * cifs_strfromUCS_le already zero terminates the string
> +                * using 1 null byte
> +                */
> +               (*dst)[dst_len+1] = 0; /* needed for Unicode (really?)*/
>         } else {
> -               plen = strnlen(src, maxlen);
> -               *dst = kmalloc(plen + 2, GFP_KERNEL);
> +               src_len = strnlen(src, maxlen);
> +               *dst = kmalloc(src_len + 1, GFP_KERNEL);
>                 if (!*dst)
> 
> cheers - Günter

Ugh. Good catch...

I probably shouldn't have tried to review that patch while heavily
jet-lagged.

Suresh, I think you need to reintroduce that UniStrnLenbytes function
here and then make cifs_strlcpy_to_host use it.

As far as the buffer size using 4 times number of wide chars:

That buffer should be sufficient for UTF-8. Are there character sets
where UCS2 chars will translate to being more than 4 bytes? If so, then
yes...we need to change it. If not, then it sounds like
NLS_MAX_CHARSET_SIZE is too big and needs to be fixed.

-- 
Jeff Layton <[email protected]>