| Message ID | 200904210424.45296.linux@kukkukk.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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.
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