linux-kernel - [PATCH] MIPS VPE support module: don't deref potentially null pbuffer and don't do pointless null check before vfree

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Sun, 7 Nov 2010 19:48:25 +0100 (CET)
From:	Jesper Juhl <jj@...osbits.net>
To:	Ralf Baechle <ralf@...ux-mips.org>
cc:	linux-mips@...ux-mips.org, linux-kernel@...r.kernel.org
Subject: [PATCH] MIPS VPE support module: don't deref potentially null pbuffer
 and don't do pointless null check before vfree

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@...osbits.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@...osbits.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.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/