Messages in this thread |  | | | Date | Thu, 18 Nov 2010 00:11:26 +0200 | | From | Alexander Shishkin <> | | Subject | Re: [PATCH RFC] ARM ETM driver: Do not deref potentially null pointer and don't allocate and free mem while holding lock. |
| |
On Sun, Nov 07, 2010 at 09:38:55PM +0100, Jesper Juhl wrote: > On Sun, 7 Nov 2010, Colin Cross wrote: > > > On Sun, Nov 7, 2010 at 11:12 AM, Jesper Juhl <jj@chaosbits.net> 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 <jj@chaosbits.net> > > > --- > > > 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 > > > > Ohh crap, you are of course right in both cases. Not being able to build > stuff sucks, gcc would have caught this for me :-( > > How about this instead?
This looks good. Thanks!
> Signed-off-by: Jesper Juhl <jj@chaosbits.net> Acked-by: Alexander Shishkin <virtuoso@slind.org>
> --- > etm.c | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/arch/arm/kernel/etm.c b/arch/arm/kernel/etm.c > index 11db628..41bd60d 100644 > --- a/arch/arm/kernel/etm.c > +++ b/arch/arm/kernel/etm.c > @@ -293,6 +293,10 @@ static ssize_t etb_read(struct file *file, char __user *data, > > length = min(total * 4, (int)len); > buf = vmalloc(length); > + if (!buf) { > + mutex_unlock(&t->mutex); > + return -ENOMEM; > + } > > dev_dbg(t->dev, "ETB buffer length: %d\n", total); > dev_dbg(t->dev, "ETB status reg: %x\n", etb_readl(t, ETBR_STATUS)); >
Regards, -- Alex
-- 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/
|  |