oss-security - additional memory leak in USB userspace handling

Openwall GNU/*/Linux - a small security-enhanced Linux distro for servers
[<prev] [next>] [thread-next>] [day] [month] [year] [list]
Date: Wed, 17 Feb 2010 11:46:19 +0100
From: Marcus Meissner <meissner@...e.de>
To: OSS Security List <oss-security@...ts.openwall.com>
Subject: additional memory leak in USB userspace handling

Hi,

a memory allocation leak (not information, just unfreed memory)
was spotted and fixed by Linus during debugging of previous problem.

On put_user() errors it would leak one "struct async" per REAPURB call.

Fix is in commit ddeee0b2eec2a51b0712b04de4b39e7bec892a53, also
attached.

Affected code is also going back throughout 2.6 history.

The issue is of less importance than the information leak fix, I am not
sure if it deserves a CVE or not.

Ciao, Marcus

commit ddeee0b2eec2a51b0712b04de4b39e7bec892a53
Author: Linus Torvalds <torvalds@...ux-foundation.org>
Date:   Tue Feb 16 12:35:07 2010 -0800

    USB: usbfs: properly clean up the as structure on error paths
    
    I notice that the processcompl_compat() function seems to be leaking the
    'struct async *as' in the error paths.
    
    I think that the calling convention is fundamentally buggered. The
    caller is the one that did the "reap_as()" to get the as thing, the
    caller should be the one to free it too.
    
    Freeing it in the caller also means that it very clearly always gets
    freed, and avoids the need for any "free in the error case too".
    
    From: Linus Torvalds <torvalds@...ux-foundation.org>
    Cc: Alan Stern <stern@...land.harvard.edu>
    Cc: Marcus Meissner <meissner@...e.de>
    Cc: stable <stable@...nel.org>
    Signed-off-by: Greg Kroah-Hartman <gregkh@...e.de>

diff --git a/drivers/usb/core/devio.c b/drivers/usb/core/devio.c
index ca948bb..a678186 100644
--- a/drivers/usb/core/devio.c
+++ b/drivers/usb/core/devio.c
@@ -1334,14 +1334,11 @@ static int processcompl(struct async *as, void __user * __user *arg)
 		}
 	}
 
-	free_async(as);
-
 	if (put_user(addr, (void __user * __user *)arg))
 		return -EFAULT;
 	return 0;
 
 err_out:
-	free_async(as);
 	return -EFAULT;
 }
 
@@ -1371,8 +1368,11 @@ static struct async *reap_as(struct dev_state *ps)
 static int proc_reapurb(struct dev_state *ps, void __user *arg)
 {
 	struct async *as = reap_as(ps);
-	if (as)
-		return processcompl(as, (void __user * __user *)arg);
+	if (as) {
+		int retval = processcompl(as, (void __user * __user *)arg);
+		free_async(as);
+		return retval;
+	}
 	if (signal_pending(current))
 		return -EINTR;
 	return -EIO;
@@ -1380,11 +1380,16 @@ static int proc_reapurb(struct dev_state *ps, void __user *arg)
 
 static int proc_reapurbnonblock(struct dev_state *ps, void __user *arg)
 {
+	int retval;
 	struct async *as;
 
-	if (!(as = async_getcompleted(ps)))
-		return -EAGAIN;
-	return processcompl(as, (void __user * __user *)arg);
+	as = async_getcompleted(ps);
+	retval = -EAGAIN;
+	if (as) {
+		retval = processcompl(as, (void __user * __user *)arg);
+		free_async(as);
+	}
+	return retval;
 }
 
 #ifdef CONFIG_COMPAT
@@ -1497,7 +1502,6 @@ static int processcompl_compat(struct async *as, void __user * __user *arg)
 		}
 	}
 
-	free_async(as);
 	if (put_user(ptr_to_compat(addr), (u32 __user *)arg))
 		return -EFAULT;
 	return 0;
@@ -1506,8 +1510,11 @@ static int processcompl_compat(struct async *as, void __user * __user *arg)
 static int proc_reapurb_compat(struct dev_state *ps, void __user *arg)
 {
 	struct async *as = reap_as(ps);
-	if (as)
-		return processcompl_compat(as, (void __user * __user *)arg);
+	if (as) {
+		int retval = processcompl_compat(as, (void __user * __user *)arg);
+		free_async(as);
+		return retval;
+	}
 	if (signal_pending(current))
 		return -EINTR;
 	return -EIO;
@@ -1515,11 +1522,16 @@ static int proc_reapurb_compat(struct dev_state *ps, void __user *arg)
 
 static int proc_reapurbnonblock_compat(struct dev_state *ps, void __user *arg)
 {
+	int retval;
 	struct async *as;
 
-	if (!(as = async_getcompleted(ps)))
-		return -EAGAIN;
-	return processcompl_compat(as, (void __user * __user *)arg);
+	retval = -EAGAIN;
+	as = async_getcompleted(ps);
+	if (as) {
+		retval = processcompl_compat(as, (void __user * __user *)arg);
+		free_async(as);
+	}
+	return retval;
 }
Please check out the Open Source Software Security Wiki, which is counterpart to this mailing list.