Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752751Ab0KGUim (ORCPT ); Sun, 7 Nov 2010 15:38:42 -0500 Received: from smtp-out.google.com ([216.239.44.51]:26682 "EHLO smtp-out.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751521Ab0KGUik convert rfc822-to-8bit (ORCPT ); Sun, 7 Nov 2010 15:38:40 -0500 DomainKey-Signature: a=rsa-sha1; c=nofws; d=google.com; s=beta; h=mime-version:sender:in-reply-to:references:date :x-google-sender-auth:message-id:subject:from:to:cc:content-type :content-transfer-encoding; b=fkqwT0L70TQG2k/anBYxrb3zflnZsI90yIvSLc6ukSdm+y9jETuCD9qQzEL/MP0ov4 L9SzKFvVzbDedbjlM+xQ== MIME-Version: 1.0 In-Reply-To: References: Date: Sun, 7 Nov 2010 12:38:35 -0800 X-Google-Sender-Auth: -kxbRIjKgyXUx6oj-hoO25_EF7Y Message-ID: Subject: Re: [PATCH RFC] ARM ETM driver: Do not deref potentially null pointer and don't allocate and free mem while holding lock. From: Colin Cross To: Jesper Juhl Cc: Alexander Shishkin , Russell King , Jason Wessel , Dmitry Torokhov , linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 8BIT X-System-Of-Record: true Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 3195 Lines: 92 On Sun, Nov 7, 2010 at 11:12 AM, Jesper Juhl wrote: > Hello, > > Looking at etb_read() in arch/arm/kernel/etm.c I noticed two things. > > ?1. We are allocting and freeing 'buf' with vmalloc() while holding a > ? ?mutex locked. I cannot see any reason why we have to hold the mutex > ? ?just to allocate and free a bit of memory, so I moved it outside the > ? ?lock. > > ?2. If the memory allocation fails we'll dereference a null pointer > ? ?further down since we never check the vmalloc() return value. > ? ? ? ?for (i = 0; i < length / 4; i++) > ? ? ? ? ? ? ? ?buf[i] = etb_readl(t, ETBR_READMEM); > ? ?The best way I could find to handle this was to simply return 0 when > ? ?we can't allocate memory, but there might be a better option that I > ? ?just couldn't find - in any case it is better than crashing the > ? ?kernel. > > Please consider merging, but please also review carefully first since I'm > not familliar with this code. > > CC me on replies please. > > > Signed-off-by: Jesper Juhl > --- > ?etm.c | ? ?8 +++++--- > ?1 file changed, 5 insertions(+), 3 deletions(-) > > ?note: completely untested patch since I have neither hardware nor > ?toolchain to test it, so please review carefully and test before applying > ?it. > > diff --git a/arch/arm/kernel/etm.c b/arch/arm/kernel/etm.c > index 11db628..30f845b 100644 > --- a/arch/arm/kernel/etm.c > +++ b/arch/arm/kernel/etm.c > @@ -274,7 +274,10 @@ static ssize_t etb_read(struct file *file, char __user *data, > ? ? ? ?long length; > ? ? ? ?struct tracectx *t = file->private_data; > ? ? ? ?u32 first = 0; > - ? ? ? u32 *buf; > + ? ? ? u32 *buf = vmalloc(length); You can't move the vmalloc out of the lock, length is uninitialized here > + > + ? ? ? if (!buf) > + ? ? ? ? ? ? ? return 0; ssize_t is signed so you can return -ENOMEM > ? ? ? ?mutex_lock(&t->mutex); > > @@ -292,7 +295,6 @@ static ssize_t etb_read(struct file *file, char __user *data, > ? ? ? ?etb_writel(t, first, ETBR_READADDR); > > ? ? ? ?length = min(total * 4, (int)len); > - ? ? ? buf = vmalloc(length); > > ? ? ? ?dev_dbg(t->dev, "ETB buffer length: %d\n", total); > ? ? ? ?dev_dbg(t->dev, "ETB status reg: %x\n", etb_readl(t, ETBR_STATUS)); > @@ -310,10 +312,10 @@ static ssize_t etb_read(struct file *file, char __user *data, > ? ? ? ?etb_lock(t); > > ? ? ? ?length -= copy_to_user(data, buf, length); > - ? ? ? vfree(buf); > > ?out: > ? ? ? ?mutex_unlock(&t->mutex); > + ? ? ? vfree(buf); > > ? ? ? ?return length; > ?} > > > > -- > Jesper Juhl ? ? ? ? ? ? http://www.chaosbits.net/ > Don't top-post ?http://www.catb.org/~esr/jargon/html/T/top-post.html > Plain text mails only, please. > > -- > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at ?http://vger.kernel.org/majordomo-info.html > Please read the FAQ at ?http://www.tux.org/lkml/ > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/