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 <[email protected]>
---
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);
+
+ if (!buf)
+ return 0;
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 <[email protected]> http://www.chaosbits.net/
Don't top-post http://www.catb.org/~esr/jargon/html/T/top-post.html
Plain text mails only, please.
On Sun, Nov 7, 2010 at 11:12 AM, Jesper Juhl <[email protected]> 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 <[email protected]>
> ---
> ?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 <[email protected]> ? ? ? ? ? ? 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 [email protected]
> More majordomo info at ?http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at ?http://www.tux.org/lkml/
>
On Sun, 7 Nov 2010, Colin Cross wrote:
> On Sun, Nov 7, 2010 at 11:12 AM, Jesper Juhl <[email protected]> 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 <[email protected]>
> > ---
> > ?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?
Signed-off-by: Jesper Juhl <[email protected]>
---
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));
--
Jesper Juhl <[email protected]> http://www.chaosbits.net/
Don't top-post http://www.catb.org/~esr/jargon/html/T/top-post.html
Plain text mails only, please.
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 <[email protected]> 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 <[email protected]>
> > > ---
> > > ?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 <[email protected]>
Acked-by: Alexander Shishkin <[email protected]>
> ---
> 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