From patchwork Tue Apr 21 07:52:03 2009 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Suresh Jayaraman X-Patchwork-Id: 19134 Received: from lists.samba.org (mail.samba.org [66.70.73.150]) by demeter.kernel.org (8.14.2/8.14.2) with ESMTP id n3L7qjiP030526 for ; Tue, 21 Apr 2009 07:52:45 GMT Received: from dp.samba.org (localhost [127.0.0.1]) by lists.samba.org (Postfix) with ESMTP id 6D88E163BE6 for ; Tue, 21 Apr 2009 07:52:23 +0000 (GMT) X-Spam-Checker-Version: SpamAssassin 3.1.7 (2006-10-05) on dp.samba.org X-Spam-Level: X-Spam-Status: No, score=-2.6 required=3.8 tests=AWL, BAYES_00 autolearn=ham version=3.1.7 X-Original-To: linux-cifs-client@lists.samba.org Delivered-To: linux-cifs-client@lists.samba.org Received: from victor.provo.novell.com (victor.provo.novell.com [137.65.250.26]) by lists.samba.org (Postfix) with ESMTP id 3449B163B69 for ; Tue, 21 Apr 2009 07:52:02 +0000 (GMT) Received: from [192.168.2.100] (prv-ext-foundry1.gns.novell.com [137.65.251.240]) by victor.provo.novell.com with ESMTP (TLS encrypted); Tue, 21 Apr 2009 01:52:17 -0600 Message-ID: <49ED7B23.80508@suse.de> Date: Tue, 21 Apr 2009 13:22:03 +0530 From: Suresh Jayaraman User-Agent: Thunderbird 2.0.0.19 (X11/20081227) MIME-Version: 1.0 To: =?UTF-8?B?R8O8bnRlciBLdWtrdWtr?= Subject: Re: [linux-cifs-client] [PATCH 2/2] cifs: Increase size of tmp_buf in cifs_readdir to avoid potential overflows References: <49EC7794.50603@suse.de> <20090420113059.122922a5@tupile.poochiereds.net> <524f69650904201258l417a081bkae1caa26e797b739@mail.gmail.com> <200904210424.45296.linux@kukkukk.com> In-Reply-To: <200904210424.45296.linux@kukkukk.com> X-Enigmail-Version: 0.95.7 Cc: Steve French , linux-cifs-client@lists.samba.org, Jeff Layton X-BeenThere: linux-cifs-client@lists.samba.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: The Linux CIFS VFS client List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: linux-cifs-client-bounces+patchwork-cifs-client=patchwork.kernel.org@lists.samba.org Errors-To: linux-cifs-client-bounces+patchwork-cifs-client=patchwork.kernel.org@lists.samba.org Günter Kukkukk wrote: > Am Montag, 20. April 2009 schrieb Steve French: >> Merged this and also patch 1 of 2 >> >> thx >> >>>> 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 >>> >> >> > > 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. Good catch, agreed, [PATCH 1/2] while only focused on allocating the right buffer size, it didn't ensure correct NULL termination of dst buffer after the change. Sorry about the oversight. Also it is correct to use the length of destination string (in bytes) while null terminating than using length of source string (in UCS chars which was the case earlier too). > 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 */ NLS_MAX_CHARSET_SIZE is a bit too much, I think. >From man utf-8: UTF-8 encoded UCS characters may be up to six bytes long, however the Unicode standard specifies no characters above 0x10ffff, so Unicode characters can only be up to four bytes long in UTF-8. There were a lot of discussion on this over the past weeks and we really wanted to have common helpers to calculate the length instead of going by assumptions. And since there are other filesystems that would need this helper the decision was to generalise the helper and make it part of common code (for e.g. fs/nls) and let other filesystems such as jfs/ntfs make use of it. This patch is an interim measure to fix the potential overflows. Also, see discussions: http://lists.samba.org/archive/linux-cifs-client/2009-April/004322.html http://lists.samba.org/archive/linux-cifs-client/2009-April/004421.html > 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) > need to pass src_len to strlcpy as well. Here is the updated patch with 2 changes (mostly nits) to Guenter's version, NLS_MAX_CHARSET_SIZE set to 4, pass src_len to strlcpy in non-unicode case. The previous patch while fixing the buffer size, left out the NULL termination that is based on length of src buffer(in UCS characters) as it is. Fix this by using the length of dst buffer got from cifs_strfromUCS_le() for NULL termination. --- fs/cifs/cifssmb.c | 21 ++++++++++++--------- 1 files changed, 12 insertions(+), 9 deletions(-) diff --git a/fs/cifs/cifssmb.c b/fs/cifs/cifssmb.c index a02c43b..9f5f7aa 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((4 * 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, src_len, + nls_codepage); + /* + * cifs_strfromUCS_le() ensures single byte NULL termination + */ + (*dst)[dst_len + 1] = 0; /* needed for Unicode, to be safe */ } 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) goto cifs_strlcpy_to_host_ErrExit; - strlcpy(*dst, src, plen); + strlcpy(*dst, src, src_len); } return 0;