[PATCH] Check vmalloc return value in vpe_open

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 <redacted>
---
 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

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

On Sun, 7 Nov 2010, Ralf Baechle wrote:

On Sat, Oct 30, 2010 at 06:37:16PM +0200, Jesper Juhl wrote:

quoted

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 <redacted>
---
 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.

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