From patchwork Thu Apr 16 17:42:05 2009 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Jeff Layton X-Patchwork-Id: 18539 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 n3GHgXfY013107 for ; Thu, 16 Apr 2009 17:42:33 GMT Received: from dp.samba.org (localhost [127.0.0.1]) by lists.samba.org (Postfix) with ESMTP id 4EFA5163D77 for ; Thu, 16 Apr 2009 17:42:12 +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=-3.8 required=3.8 tests=AWL, BAYES_00, SPF_HELO_PASS, SPF_PASS 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 mx2.redhat.com (mx2.redhat.com [66.187.237.31]) by lists.samba.org (Postfix) with ESMTP id 05C72163BDE for ; Thu, 16 Apr 2009 17:41:51 +0000 (GMT) Received: from int-mx2.corp.redhat.com (int-mx2.corp.redhat.com [172.16.27.26]) by mx2.redhat.com (8.13.8/8.13.8) with ESMTP id n3GHg7tH018146; Thu, 16 Apr 2009 13:42:07 -0400 Received: from ns3.rdu.redhat.com (ns3.rdu.redhat.com [10.11.255.199]) by int-mx2.corp.redhat.com (8.13.1/8.13.1) with ESMTP id n3GHg72T017139; Thu, 16 Apr 2009 13:42:07 -0400 Received: from barsoom.rdu.redhat.com (barsoom.rdu.redhat.com [10.11.228.36]) by ns3.rdu.redhat.com (8.13.8/8.13.8) with ESMTP id n3GHg65B030483; Thu, 16 Apr 2009 13:42:06 -0400 Date: Thu, 16 Apr 2009 13:42:05 -0400 From: Jeff Layton To: Dave Kleikamp Subject: Re: [linux-cifs-client] [PATCH 1/2] cifs: fix buffer size for tcon->nativeFileSystem field Message-ID: <20090416134205.2840ba1c@barsoom.rdu.redhat.com> In-Reply-To: <1239896493.7456.5.camel@norville.austin.ibm.com> References: <1239895313-11841-1-git-send-email-jlayton@redhat.com> <1239896493.7456.5.camel@norville.austin.ibm.com> Mime-Version: 1.0 X-Scanned-By: MIMEDefang 2.58 on 172.16.27.26 Cc: smfrench@gmail.com, linux-cifs-client@lists.samba.org, sjayaraman@suse.de 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 On Thu, 16 Apr 2009 15:41:33 +0000 Dave Kleikamp wrote: > On Thu, 2009-04-16 at 11:21 -0400, Jeff Layton wrote: > > The buffer for this was resized recently to fix a bug. It's still > > possible however that a malicious server could overflow this field > > by sending characters in it that are >2 bytes in the local charset. > > Double the size of the buffer to account for this possibility. > > > > Also get rid of some really strange and seemingly pointless NULL > > termination. It's NULL terminating the string in the source buffer, > > but by the time that happens, we've already copied the string. > > > > Signed-off-by: Jeff Layton > > --- > > fs/cifs/connect.c | 7 ++----- > > 1 files changed, 2 insertions(+), 5 deletions(-) > > > > diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c > > index 01e280c..1a93604 100644 > > --- a/fs/cifs/connect.c > > +++ b/fs/cifs/connect.c > > @@ -3756,16 +3756,13 @@ CIFSTCon(unsigned int xid, struct cifsSesInfo *ses, > > BCC(smb_buffer_response)) { > > kfree(tcon->nativeFileSystem); > > tcon->nativeFileSystem = > > - kzalloc(2*(length + 1), GFP_KERNEL); > > + kzalloc((4 * length) + 2, GFP_KERNEL); > > if (tcon->nativeFileSystem) > > cifs_strfromUCS_le( > > tcon->nativeFileSystem, > > (__le16 *) bcc_ptr, > > length, nls_codepage); > > - bcc_ptr += 2 * length; > > - bcc_ptr[0] = 0; /* null terminate the string */ > > - bcc_ptr[1] = 0; > > - bcc_ptr += 2; > > + bcc_ptr += (2 * length) + 2; > > What's the point of updating bcc_ptr here? It's not accurate anyway. > The correct thing would be: > > bcc_ptr += cifs_strfromUCS_le(... ); > > but bcc_ptr isn't used again, so there's no point. > > Shaggy > -- > David Kleikamp > IBM Linux Technology Center Here's a respun patch that removes the bcc_ptr update. >From d4a32dbdeb49a77536744a16ddce6e04e0af9592 Mon Sep 17 00:00:00 2001 From: Jeff Layton Date: Thu, 16 Apr 2009 13:34:59 -0400 Subject: [PATCH] cifs: fix buffer size for tcon->nativeFileSystem field The buffer for this was resized recently to fix a bug. It's still possible however that a malicious server could overflow this field by sending characters in it that are >2 bytes in the local charset. Double the size of the buffer to account for this possibility. Also get rid of some really strange and seemingly pointless NULL termination. It's NULL terminating the string in the source buffer, but by the time that happens, we've already copied the string. Signed-off-by: Jeff Layton --- fs/cifs/connect.c | 6 +----- 1 files changed, 1 insertions(+), 5 deletions(-) diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c index 01e280c..9cdc99c 100644 --- a/fs/cifs/connect.c +++ b/fs/cifs/connect.c @@ -3756,16 +3756,12 @@ CIFSTCon(unsigned int xid, struct cifsSesInfo *ses, BCC(smb_buffer_response)) { kfree(tcon->nativeFileSystem); tcon->nativeFileSystem = - kzalloc(2*(length + 1), GFP_KERNEL); + kzalloc((4 * length) + 2, GFP_KERNEL); if (tcon->nativeFileSystem) cifs_strfromUCS_le( tcon->nativeFileSystem, (__le16 *) bcc_ptr, length, nls_codepage); - bcc_ptr += 2 * length; - bcc_ptr[0] = 0; /* null terminate the string */ - bcc_ptr[1] = 0; - bcc_ptr += 2; } /* else do not bother copying these information fields*/ } else { -- 1.5.5.6