Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754779Ab0H3I66 (ORCPT ); Mon, 30 Aug 2010 04:58:58 -0400 Received: from he.sipsolutions.net ([78.46.109.217]:49570 "EHLO sipsolutions.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751548Ab0H3I64 (ORCPT ); Mon, 30 Aug 2010 04:58:56 -0400 Subject: Re: [PATCH] wireless: fix 64K kernel heap content leak via ioctl From: Johannes Berg To: jt@hpl.hp.com Cc: Kees Cook , linux-kernel@vger.kernel.org, "John W. Linville" , "David S. Miller" , Eric Dumazet , Joe Perches , Tejun Heo , linux-wireless@vger.kernel.org, netdev@vger.kernel.org In-Reply-To: <1283158058.3691.10.camel@jlt3.sipsolutions.net> References: <20100827210240.GC4703@outflux.net> <20100827212254.GB32275@bougret.hpl.hp.com> <1283158058.3691.10.camel@jlt3.sipsolutions.net> Content-Type: text/plain; charset="UTF-8" Date: Mon, 30 Aug 2010 10:58:24 +0200 Message-ID: <1283158704.3691.11.camel@jlt3.sipsolutions.net> Mime-Version: 1.0 X-Mailer: Evolution 2.30.3 Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 3856 Lines: 99 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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/