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

[Johannes Berg]

On Mon, 2010-08-30 at 10:47 +0200, Johannes Berg wrote:
> 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!!)

err, forgot quilt refresh, here's the right patch:

---
 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:57:38.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 linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/