From mboxrd@z Thu Jan 1 00:00:00 1970 From: Steve French Subject: Re: [PATCH 1/1] cifs: potential memory leaks when parsing mnt opts Date: Mon, 23 Mar 2015 23:43:55 -0500 Message-ID: References: <1426979310-31201-1-git-send-email-tsgatesv@gmail.com> Mime-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Cc: Steve French , "linux-cifs-u79uwXL29TY76Z2rM5mHXA@public.gmane.org" , samba-technical , LKML , taesoo-/4noJB3qBVQ3uPMLIKxrzw@public.gmane.org, changwoo-/4noJB3qBVQ3uPMLIKxrzw@public.gmane.org, sanidhya-/4noJB3qBVQ3uPMLIKxrzw@public.gmane.org, blee-/4noJB3qBVQ3uPMLIKxrzw@public.gmane.org, csong84-/4noJB3qBVQ3uPMLIKxrzw@public.gmane.org, Scott Lovenberg To: Taesoo Kim Return-path: In-Reply-To: <1426979310-31201-1-git-send-email-tsgatesv-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> Sender: linux-cifs-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org List-ID: Nice catch. Merged into cifs-2.6.git for-next On Sat, Mar 21, 2015 at 6:08 PM, Taesoo Kim wrote: > For example, when mount opt is redundently specified > (e.g., "user=A,user=B,user=C"), kernel kept allocating new key/val > with kstrdup() and overwrite previous ptr (to be freed). > > Althouhg mkfs.cifs in userspace performs a bit of sanitization > (e.g., forcing one user option), current implementation is not > robust. Other options such as iocharset and domainanme are similary > vulnerable. > > Signed-off-by: Taesoo Kim > --- > fs/cifs/connect.c | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c > index d3aa999..4cb8450 100644 > --- a/fs/cifs/connect.c > +++ b/fs/cifs/connect.c > @@ -1599,6 +1599,8 @@ cifs_parse_mount_options(const char *mountdata, const char *devname, > pr_warn("CIFS: username too long\n"); > goto cifs_parse_mount_err; > } > + > + kfree(vol->username); > vol->username = kstrdup(string, GFP_KERNEL); > if (!vol->username) > goto cifs_parse_mount_err; > @@ -1700,6 +1702,7 @@ cifs_parse_mount_options(const char *mountdata, const char *devname, > goto cifs_parse_mount_err; > } > > + kfree(vol->domainname); > vol->domainname = kstrdup(string, GFP_KERNEL); > if (!vol->domainname) { > pr_warn("CIFS: no memory for domainname\n"); > @@ -1731,6 +1734,7 @@ cifs_parse_mount_options(const char *mountdata, const char *devname, > } > > if (strncasecmp(string, "default", 7) != 0) { > + kfree(vol->iocharset); > vol->iocharset = kstrdup(string, > GFP_KERNEL); > if (!vol->iocharset) { > -- > 2.3.3 > -- Thanks, Steve From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753085AbbCXEoS (ORCPT ); Tue, 24 Mar 2015 00:44:18 -0400 Received: from mail-qg0-f52.google.com ([209.85.192.52]:36666 "EHLO mail-qg0-f52.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750902AbbCXEoQ (ORCPT ); Tue, 24 Mar 2015 00:44:16 -0400 MIME-Version: 1.0 In-Reply-To: <1426979310-31201-1-git-send-email-tsgatesv@gmail.com> References: <1426979310-31201-1-git-send-email-tsgatesv@gmail.com> From: Steve French Date: Mon, 23 Mar 2015 23:43:55 -0500 Message-ID: Subject: Re: [PATCH 1/1] cifs: potential memory leaks when parsing mnt opts To: Taesoo Kim Cc: Steve French , "linux-cifs@vger.kernel.org" , samba-technical , LKML , taesoo@gatech.edu, changwoo@gatech.edu, sanidhya@gatech.edu, blee@gatech.edu, csong84@gatech.edu, Scott Lovenberg Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Nice catch. Merged into cifs-2.6.git for-next On Sat, Mar 21, 2015 at 6:08 PM, Taesoo Kim wrote: > For example, when mount opt is redundently specified > (e.g., "user=A,user=B,user=C"), kernel kept allocating new key/val > with kstrdup() and overwrite previous ptr (to be freed). > > Althouhg mkfs.cifs in userspace performs a bit of sanitization > (e.g., forcing one user option), current implementation is not > robust. Other options such as iocharset and domainanme are similary > vulnerable. > > Signed-off-by: Taesoo Kim > --- > fs/cifs/connect.c | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c > index d3aa999..4cb8450 100644 > --- a/fs/cifs/connect.c > +++ b/fs/cifs/connect.c > @@ -1599,6 +1599,8 @@ cifs_parse_mount_options(const char *mountdata, const char *devname, > pr_warn("CIFS: username too long\n"); > goto cifs_parse_mount_err; > } > + > + kfree(vol->username); > vol->username = kstrdup(string, GFP_KERNEL); > if (!vol->username) > goto cifs_parse_mount_err; > @@ -1700,6 +1702,7 @@ cifs_parse_mount_options(const char *mountdata, const char *devname, > goto cifs_parse_mount_err; > } > > + kfree(vol->domainname); > vol->domainname = kstrdup(string, GFP_KERNEL); > if (!vol->domainname) { > pr_warn("CIFS: no memory for domainname\n"); > @@ -1731,6 +1734,7 @@ cifs_parse_mount_options(const char *mountdata, const char *devname, > } > > if (strncasecmp(string, "default", 7) != 0) { > + kfree(vol->iocharset); > vol->iocharset = kstrdup(string, > GFP_KERNEL); > if (!vol->iocharset) { > -- > 2.3.3 > -- Thanks, Steve