diff mbox

wireless: fix 64K kernel heap content leak via ioctl

Message ID 1283162341.3691.45.camel@jlt3.sipsolutions.net
State Not Applicable, archived
Delegated to: David Miller
Headers show

Commit Message

Johannes Berg Aug. 30, 2010, 9:59 a.m. UTC 
On Mon, 2010-08-30 at 10:58 +0200, Johannes Berg wrote:

> > > @@ -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;

Ok I finally fully understood the issue.

This will fix the problem, but the comment is completely bogus, which I
guess means you didn't actually understand the problem.

> > 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!!)

My patch also didn't fix the problem, I didn't understand the problem
correctly and was continuously wondering how drivers would ever fill the
buffer with more than max_tokens (which would be a more serious bug,
since they'd overwrite a slab object after "extra").

What really fixes the problem is the patch below though. Had to realise
that the path where the driver didn't do ANYTHING AT ALL was the
problem....

johannes

Subject: wireless extensions: fix kernel heap content leak

From: Johannes Berg <johannes.berg@intel.com>

Wireless extensions have an unfortunate, undocumented
requirement which requires drivers to always fill
iwp->length when returning a successful status. When
a driver doesn't do this, it leads to a kernel heap
content leak when userspace offers a larger buffer
than would have been necessary.

Arguably, this is a driver bug, as it should, if it
returns 0, fill iwp->length, even if it separately
indicated that the buffer contents was not valid.

However, we can also remove this requirement and
avoid this class of driver bugs by setting the iwp
length to max_tokens, which then reflects how big
the buffer is that the driver may fill, regardless
of how big the userspace buffer is.

To illustrate the point, this patch also fixes a
corresponding cfg80211 bug (since this requirement
isn't documented nor was ever pointed out by anyone
during code review, I don't trust all drivers nor
all cfg80211 handlers to implement it correctly).

Signed-off-by: Johannes Berg <johannes.berg@intel.com>
---
 net/wireless/wext-compat.c |    3 +++
 net/wireless/wext-core.c   |   16 ++++++++++++++++
 2 files changed, 19 insertions(+)



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

Comments

Jean Tourrilhes Aug. 30, 2010, 5:40 p.m. UTC | #1 
On Mon, Aug 30, 2010 at 11:59:01AM +0200, Johannes Berg wrote:
> 
> Ok I finally fully understood the issue.
> 
> This will fix the problem, but the comment is completely bogus, which I
> guess means you didn't actually understand the problem.

	Correct, Kees pointed out that my comment was bogus and the
e-mail I sent after the patch corrected myself on that point :

------------------------------------
> The comment should probably be clarified -- it's the caller's iwp->length
> that may be causing problems

        Ha ! I see. It would be for regular iwpoint queries, not for
extended NOMAX queries (scan is a extended NOMAX query).
------------------------------------

> My patch also didn't fix the problem, I didn't understand the problem
> correctly and was continuously wondering how drivers would ever fill the
> buffer with more than max_tokens (which would be a more serious bug,
> since they'd overwrite a slab object after "extra").

	Yes, I had arrived at the same conclusion (not that my patch
did fix the issue).

> What really fixes the problem is the patch below though. Had to realise
> that the path where the driver didn't do ANYTHING AT ALL was the
> problem....

	I actually like your patch better than mine, it's closer to
the original intent of the API. Go for it ;-)

> johannes

	Thanks a lot for the second pair of eyes.

	Jean
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Johannes Berg Aug. 30, 2010, 5:50 p.m. UTC | #2 
On Mon, 2010-08-30 at 10:40 -0700, Jean Tourrilhes wrote:

>         Ha ! I see. It would be for regular iwpoint queries, not for
> extended NOMAX queries (scan is a extended NOMAX query).

Yeah, that took a while to sink in here too ..

> 	I actually like your patch better than mine, it's closer to
> the original intent of the API. Go for it ;-)

> 	Thanks a lot for the second pair of eyes.

And thank you for looking over my patch. I guess the real bug is still
in cfg80211 there.

Initially I considered setting iwp->length to zero, rather than
max_tokens. What do you think about doing that? The reason I decided to
use max_tokens was that somebody might look at the length value to know
how much to copy (which is OK only in NOMAX queries); but that would be
a more severe driver bug ... can't really make up my mind. We just copy
back zeroes anyway.

johannes

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

Patch

--- wireless-testing.orig/net/wireless/wext-core.c	2010-08-30 11:29:26.000000000 +0200
+++ wireless-testing/net/wireless/wext-core.c	2010-08-30 11:57:41.000000000 +0200
@@ -782,6 +782,22 @@  static int ioctl_standard_iw_point(struc
 		}
 	}
 
+	if (IW_IS_GET(cmd) && !(descr->flags & IW_DESCR_FLAG_NOMAX)) {
+		/*
+		 * If this is a GET, but not NOMAX, it means that the extra
+		 * data is not bounded by userspace, but by max_tokens. Thus
+		 * set the length to max_tokens. This matches the extra data
+		 * allocation.
+		 * The driver should fill it with the number of tokens it
+		 * provided, and it may check iwp->length rather than having
+		 * knowledge of max_tokens. If the driver doesn't change the
+		 * iwp->length, this ioctl just copies back max_token tokens
+		 * filled with zeroes. Hopefully the driver isn't claiming
+		 * them to be valid data.
+		 */
+		iwp->length = descr->max_tokens;
+	}
+
 	err = handler(dev, info, (union iwreq_data *) iwp, extra);
 
 	iwp->length += essid_compat;
--- wireless-testing.orig/net/wireless/wext-compat.c	2010-08-30 11:49:22.000000000 +0200
+++ wireless-testing/net/wireless/wext-compat.c	2010-08-30 11:49:29.000000000 +0200
@@ -1420,6 +1420,9 @@  int cfg80211_wext_giwessid(struct net_de
 {
 	struct wireless_dev *wdev = dev->ieee80211_ptr;
 
+	data->flags = 0;
+	data->length = 0;
+
 	switch (wdev->iftype) {
 	case NL80211_IFTYPE_ADHOC:
 		return cfg80211_ibss_wext_giwessid(dev, info, data, ssid);