* [PATCH] Check vmalloc return value in vpe_open
@ 2010-10-30 16:37 Jesper Juhl
2010-11-07 14:29 ` Ralf Baechle
0 siblings, 1 reply; 4+ messages in thread
From: Jesper Juhl @ 2010-10-30 16:37 UTC (permalink / raw)
To: Ralf Baechle; +Cc: linux-mips, linux-kernel
Hi,
I noticed that the return value of the
vmalloc() call in arch/mips/kernel/vpe.c::vpe_open() is not checked, so we
potentially store a null pointer in v->pbuffer. As far as I can tell this
will be a problem. However, I don't know the mips code at all, so there
may be something, somewhere where I did not look, that handles this in a
safe manner but I couldn't find it.
To me it looks like we should do what the patch below implements and check
for a null return and then return -ENOMEM in that case. Comments?
Please CC me on replies as I'm not subscribed to linux-mips.
Signed-off-by: Jesper Juhl <jj@chaosbits.net>
---
vpe.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/arch/mips/kernel/vpe.c b/arch/mips/kernel/vpe.c
index 3eb3cde..22b79f6 100644
--- a/arch/mips/kernel/vpe.c
+++ b/arch/mips/kernel/vpe.c
@@ -1092,6 +1092,10 @@ static int vpe_open(struct inode *inode, struct file *filp)
/* this of-course trashes what was there before... */
v->pbuffer = vmalloc(P_SIZE);
+ if (!v->pbuffer) {
+ pr_warning("VPE loader: unable to allocate memory\n");
+ return -ENOMEM;
+ }
v->plen = P_SIZE;
v->load_addr = NULL;
v->len = 0;
--
Jesper Juhl <jj@chaosbits.net> http://www.chaosbits.net/
Plain text mails only, please http://www.expita.com/nomime.html
Don't top-post http://www.catb.org/~esr/jargon/html/T/top-post.html
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] Check vmalloc return value in vpe_open
2010-10-30 16:37 [PATCH] Check vmalloc return value in vpe_open Jesper Juhl
@ 2010-11-07 14:29 ` Ralf Baechle
2010-11-07 18:48 ` [PATCH] MIPS VPE support module: don't deref potentially null pbuffer and don't do pointless null check before vfree Jesper Juhl
0 siblings, 1 reply; 4+ messages in thread
From: Ralf Baechle @ 2010-11-07 14:29 UTC (permalink / raw)
To: Jesper Juhl; +Cc: linux-mips, linux-kernel
On Sat, Oct 30, 2010 at 06:37:16PM +0200, Jesper Juhl wrote:
> I noticed that the return value of the
> vmalloc() call in arch/mips/kernel/vpe.c::vpe_open() is not checked, so we
> potentially store a null pointer in v->pbuffer. As far as I can tell this
> will be a problem. However, I don't know the mips code at all, so there
> may be something, somewhere where I did not look, that handles this in a
> safe manner but I couldn't find it.
>
> To me it looks like we should do what the patch below implements and check
> for a null return and then return -ENOMEM in that case. Comments?
All users check if the buffer was successfully allocated so the code is
safe wrt. to that.
Doesn't mean that it's not a pukeogenic piece of code. Look at the use of
v->pbuffer in vpe_release for example. First use it the vmalloc'ed memory
then carefully check the pointer for being non-NULL before calling vfree.
If the pointer could actually be non-NULL that's too late and vfree does
that check itself anyway. And more such gems, general uglyness and
freedom of concept. It used to be even worse.
Ralf
^ permalink raw reply [flat|nested] 4+ messages in thread
* [PATCH] MIPS VPE support module: don't deref potentially null pbuffer and don't do pointless null check before vfree
2010-11-07 14:29 ` Ralf Baechle
@ 2010-11-07 18:48 ` Jesper Juhl
2010-11-08 22:44 ` Ralf Baechle
0 siblings, 1 reply; 4+ messages in thread
From: Jesper Juhl @ 2010-11-07 18:48 UTC (permalink / raw)
To: Ralf Baechle; +Cc: linux-mips, linux-kernel
On Sun, 7 Nov 2010, Ralf Baechle wrote:
> On Sat, Oct 30, 2010 at 06:37:16PM +0200, Jesper Juhl wrote:
>
> > I noticed that the return value of the
> > vmalloc() call in arch/mips/kernel/vpe.c::vpe_open() is not checked, so we
> > potentially store a null pointer in v->pbuffer. As far as I can tell this
> > will be a problem. However, I don't know the mips code at all, so there
> > may be something, somewhere where I did not look, that handles this in a
> > safe manner but I couldn't find it.
> >
> > To me it looks like we should do what the patch below implements and check
> > for a null return and then return -ENOMEM in that case. Comments?
>
> All users check if the buffer was successfully allocated so the code is
> safe wrt. to that.
>
> Doesn't mean that it's not a pukeogenic piece of code. Look at the use of
> v->pbuffer in vpe_release for example. First use it the vmalloc'ed memory
> then carefully check the pointer for being non-NULL before calling vfree.
> If the pointer could actually be non-NULL that's too late and vfree does
> that check itself anyway. And more such gems, general uglyness and
> freedom of concept. It used to be even worse.
>
Thanks for looking at the patch and commenting.
I've taken a second look. I still have no way at all to test this, so
please take a close look before potentially applying it, but how does this
look to you?
Don't dereference pbuffer before ttesting it for null.
Don't do pointless check of pointer passed to vfree for null as vfree does
this itself.
Signed-off-by: Jesper Juhl <jj@chaosbits.net>
---
vpe.c | 11 +++++++++--
1 file changed, 9 insertions(+), 2 deletions(-)
diff --git a/arch/mips/kernel/vpe.c b/arch/mips/kernel/vpe.c
index 3eb3cde..e22f258 100644
--- a/arch/mips/kernel/vpe.c
+++ b/arch/mips/kernel/vpe.c
@@ -854,6 +854,9 @@ static int vpe_elfload(struct vpe * v)
strcpy(mod.name, "VPE loader");
hdr = (Elf_Ehdr *) v->pbuffer;
+ if (!hdr)
+ return -ENOMEM;
+
len = v->plen;
/* Sanity checks against insmoding binaries or wrong arch,
@@ -1129,6 +1132,10 @@ static int vpe_release(struct inode *inode, struct file *filp)
return -ENODEV;
hdr = (Elf_Ehdr *) v->pbuffer;
+ if (!hdr) {
+ ret = -ENOMEM;
+ goto out;
+ }
if (memcmp(hdr->e_ident, ELFMAG, SELFMAG) == 0) {
if (vpe_elfload(v) >= 0) {
vpe_run(v);
@@ -1141,6 +1148,7 @@ static int vpe_release(struct inode *inode, struct file *filp)
ret = -ENOEXEC;
}
+ out:
/* It's good to be able to run the SP and if it chokes have a look at
the /dev/rt?. But if we reset the pointer to the shared struct we
lose what has happened. So perhaps if garbage is sent to the vpe
@@ -1150,8 +1158,7 @@ static int vpe_release(struct inode *inode, struct file *filp)
v->shared_ptr = NULL;
// cleanup any temp buffers
- if (v->pbuffer)
- vfree(v->pbuffer);
+ vfree(v->pbuffer);
v->plen = 0;
return ret;
}
--
Jesper Juhl <jj@chaosbits.net> http://www.chaosbits.net/
Don't top-post http://www.catb.org/~esr/jargon/html/T/top-post.html
Plain text mails only, please.
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] MIPS VPE support module: don't deref potentially null pbuffer and don't do pointless null check before vfree
2010-11-07 18:48 ` [PATCH] MIPS VPE support module: don't deref potentially null pbuffer and don't do pointless null check before vfree Jesper Juhl
@ 2010-11-08 22:44 ` Ralf Baechle
0 siblings, 0 replies; 4+ messages in thread
From: Ralf Baechle @ 2010-11-08 22:44 UTC (permalink / raw)
To: Jesper Juhl; +Cc: linux-mips, linux-kernel
On Sun, Nov 07, 2010 at 07:48:25PM +0100, Jesper Juhl wrote:
> I've taken a second look. I still have no way at all to test this, so
> please take a close look before potentially applying it, but how does this
> look to you?
Testing is a little tricky - it requires a MIPS 34K processor in a
configuration with more than one VPE.
> Don't dereference pbuffer before ttesting it for null.
> Don't do pointless check of pointer passed to vfree for null as vfree does
> this itself.
I've already checked in something based on your first patch with the
other changes I suggested added.
Thanks!
Ralf
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2010-11-09 4:24 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2010-10-30 16:37 [PATCH] Check vmalloc return value in vpe_open Jesper Juhl
2010-11-07 14:29 ` Ralf Baechle
2010-11-07 18:48 ` [PATCH] MIPS VPE support module: don't deref potentially null pbuffer and don't do pointless null check before vfree Jesper Juhl
2010-11-08 22:44 ` Ralf Baechle
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).