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

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