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

Johannes Berg
2010-08-30T08:58:58Z

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. */