From patchwork Tue Apr 21 02:24:43 2009 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: =?utf-8?b?R8ODwrxudGVyIEt1a2t1a2s=?= X-Patchwork-Id: 19114 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 n3L2P0fT007790 for ; Tue, 21 Apr 2009 02:25:01 GMT Received: from dp.samba.org (localhost [127.0.0.1]) by lists.samba.org (Postfix) with ESMTP id E1242163C4C for ; Tue, 21 Apr 2009 02:24:38 +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=-1.9 required=3.8 tests=AWL,BAYES_00, FORGED_RCVD_HELO, NO_MORE_FUNN, SPF_HELO_PASS autolearn=no version=3.1.7 X-Original-To: linux-cifs-client@lists.samba.org Delivered-To: linux-cifs-client@lists.samba.org Received: from moutng.kundenserver.de (moutng.kundenserver.de [212.227.126.187]) by lists.samba.org (Postfix) with ESMTP id 70480163B6E for ; Tue, 21 Apr 2009 02:24:07 +0000 (GMT) Received: from linux300.intranet01.hom (p57A08237.dip0.t-ipconnect.de [87.160.130.55]) by mrelayeu.kundenserver.de (node=mrelayeu7) with ESMTP (Nemesis) id 0ML2xA-1Lw5et2BgS-0001Zt; Tue, 21 Apr 2009 04:24:27 +0200 From: =?iso-8859-1?q?G=FCnter_Kukkukk?= Organization: =?utf-8?q?Entwicklungsb=C3=BCro_f=C3=BCr?= Informationstechnologien To: linux-cifs-client@lists.samba.org Subject: Re: [linux-cifs-client] [PATCH 2/2] cifs: Increase size of tmp_buf in cifs_readdir to avoid potential overflows Date: Tue, 21 Apr 2009 04:24:43 +0200 User-Agent: KMail/1.9.6 (enterprise 20070904.708012) References: <49EC7794.50603@suse.de> <20090420113059.122922a5@tupile.poochiereds.net> <524f69650904201258l417a081bkae1caa26e797b739@mail.gmail.com> In-Reply-To: <524f69650904201258l417a081bkae1caa26e797b739@mail.gmail.com> MIME-Version: 1.0 Content-Disposition: inline Message-Id: <200904210424.45296.linux@kukkukk.com> X-Provags-ID: V01U2FsdGVkX18aV3DidPGw6Tak5oTWA0hllfxiu1Bz6I5auND Di5R4KFzjWchp1d7tmClh+IpsYGhF2pkQ5qFe6xXhN3e7V6MMZ 8OEhFi2BviSVsKB2RcmkkjVJ+NTn7zb Cc: Steve French , Suresh Jayaraman , 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 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 wrote: > > On Mon, 20 Apr 2009 18:54:36 +0530 > > Suresh Jayaraman wrote: > > > >> Increase size of tmp_buf to possible maximum to avoid potential > >> overflows. > >> > >> > >> Pointed-out-by: Jeff Layton > >> Signed-off-by: Suresh Jayaraman > >> --- > >>  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 > > > > > 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 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