netdev - Re: [PATCH] wireless: fix 64K kernel heap content leak via ioctl

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]

Message-ID: <1283158058.3691.10.camel@jlt3.sipsolutions.net>
Date:	Mon, 30 Aug 2010 10:47:38 +0200
From:	Johannes Berg <johannes@...solutions.net>
To:	jt@....hp.com
Cc:	Kees Cook <kees.cook@...onical.com>, linux-kernel@...r.kernel.org,
	"John W. Linville" <linville@...driver.com>,
	"David S. Miller" <davem@...emloft.net>,
	Eric Dumazet <eric.dumazet@...il.com>,
	Joe Perches <joe@...ches.com>, Tejun Heo <tj@...nel.org>,
	linux-wireless@...r.kernel.org, netdev@...r.kernel.org
Subject: Re: [PATCH] wireless: fix 64K kernel heap content leak via ioctl

On Fri, 2010-08-27 at 14:22 -0700, Jean Tourrilhes wrote:

> 	Would you mind validating the following patch ? I've just
> verified that it compiles and I believe it does what you are asking in
> a much more predictable way.
> 	Regards,


> @@ -800,9 +800,12 @@ static int ioctl_standard_iw_point(struc
>                         goto out;
>                 }
>  
> -               if (copy_to_user(iwp->pointer, extra,
> -                                iwp->length *
> -                                descr->token_size)) {
> +               /* Verify how much we should return. Some driver
> +                * may abuse iwp->length... */
> +               if((iwp->length * descr->token_size) < extra_size)
> +                       extra_size = iwp->length * descr->token_size;
> +
> +               if (copy_to_user(iwp->pointer, extra, extra_size)) {
>                         err = -EFAULT;
>                         goto out;

Based on the code _before_ this hunk, I believe this patch to be wrong
(the goto out matches):

        /* If we have something to return to the user */
        if (!err && IW_IS_GET(cmd)) {
                /* Check if there is enough buffer up there */
                if (user_length < iwp->length) {
                        err = -E2BIG;
                        goto out;
                }

Thus, apparently drivers were intended to be allowed to return more
information than userspace had allocated space for (which also matches
the initial extra_size calculation in this function), so your comment is
wrong, and your check is also wrong because you actually put the burden
on the driver, contrary to the apparent intention of this code.

I believe the below patch is a much better fix as it allows the -E2BIG
code path to be invoked which is more informative to users than
truncated information (which, in your code, may even be truncated in the
middle of a token!!)

johannes

---
 net/wireless/wext-core.c |   23 ++++++++++-------------
 1 file changed, 10 insertions(+), 13 deletions(-)

--- wireless-testing.orig/net/wireless/wext-core.c	2010-08-30 10:35:33.000000000 +0200
+++ wireless-testing/net/wireless/wext-core.c	2010-08-30 10:46:45.000000000 +0200
@@ -738,26 +738,23 @@ static int ioctl_standard_iw_point(struc
 		/* Save user space buffer size for checking */
 		user_length = iwp->length;
 
-		/* Don't check if user_length > max to allow forward
-		 * compatibility. The test user_length < min is
-		 * implied by the test at the end.
-		 */
-
 		/* Support for very large requests */
-		if ((descr->flags & IW_DESCR_FLAG_NOMAX) &&
-		    (user_length > descr->max_tokens)) {
+		if (descr->flags & IW_DESCR_FLAG_NOMAX) {
 			/* Allow userspace to GET more than max so
 			 * we can support any size GET requests.
-			 * There is still a limit : -ENOMEM.
-			 */
-			extra_size = user_length * descr->token_size;
-
-			/* Note : user_length is originally a __u16,
+			 * There is still a limit: 64k records of
+			 * token_size (since a u16 is used).
+			 *
+			 * Note: user_length is originally a u16,
 			 * and token_size is controlled by us,
 			 * so extra_size won't get negative and
 			 * won't overflow...
 			 */
-		}
+			if (user_length > descr->max_tokens) {
+				extra_size = user_length * descr->token_size;
+		} else
+			user_length = min_t(int, user_length,
+						 descr->max_tokens);
 	}
 
 	/* kzalloc() ensures NULL-termination for essid_compat. */


--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html