[linux-cifs-client,2/2] cifs: Increase size of tmp_buf in cifs_readdir to avoid potential overflows
diff mbox

Message ID 200904210424.45296.linux@kukkukk.com
State New, archived
Headers show

Commit Message

Günter Kukkukk April 21, 2009, 2:24 a.m. UTC
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 <jlayton@redhat.com> wrote:
> > On Mon, 20 Apr 2009 18:54:36 +0530
> > Suresh Jayaraman <sjayaraman@suse.de> wrote:
> >
> >> Increase size of tmp_buf to possible maximum to avoid potential
> >> overflows.
> >>
> >>
> >> Pointed-out-by: Jeff Layton <jlayton@redhat.com>
> >> Signed-off-by: Suresh Jayaraman <sjayaraman@suse.de>
> >> ---
> >>  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 <jlayton@redhat.com>
> >
> 
> 
> 

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:
https://lists.samba.org/mailman/listinfo/linux-cifs-client

Comments

Jeff Layton April 21, 2009, 8:17 a.m. UTC | #1
On Tue, 21 Apr 2009 04:24:43 +0200
Günter Kukkukk <linux@kukkukk.com> 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 <jlayton@redhat.com> wrote:
> > > On Mon, 20 Apr 2009 18:54:36 +0530
> > > Suresh Jayaraman <sjayaraman@suse.de> wrote:
> > >
> > >> Increase size of tmp_buf to possible maximum to avoid potential
> > >> overflows.
> > >>
> > >>
> > >> Pointed-out-by: Jeff Layton <jlayton@redhat.com>
> > >> Signed-off-by: Suresh Jayaraman <sjayaraman@suse.de>
> > >> ---
> > >>  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 <jlayton@redhat.com>
> > >
> > 
> > 
> > 
> 
> 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.

Patch
diff mbox

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
_______________________________________________
linux-cifs-client mailing list
linux-cifs-client@lists.samba.org