[PATCH 0/3] wext: fix an ancient bug

STALE3409d

5 messages, 2 authors, 2017-06-14 · open the first message on its own page

[PATCH 0/3] wext: fix an ancient bug

From: Johannes Berg <johannes@sipsolutions.net>
Date: 2017-06-14 11:55:43

Robert O'Callahan reported that since struct iwreq is smaller than
struct ifreq, if it happens to land at the end of a mapped section
userspace will erroneously get -EFAULT.

Bug report is https://bugzilla.kernel.org/show_bug.cgi?id=195869.

This bug is ancient, it kernel 2.1.15 was the first to contain the
bug, and was released almost exactly 20.5 years ago.

Dave, I don't know if we should even bother sending this to stable,
but if so, the third patch will apply by itself but not compile, a
smaller patch consist of only the net/core/dev_ioctl.c modifications
with a cast thrown in where the compile fails/warns due to passing
the wrong struct type.

Should I merge this through my tree?

johannes

[PATCH 1/3] wireless: wext: remove ndo_do_ioctl fallback

From: Johannes Berg <johannes@sipsolutions.net>
Date: 2017-06-14 11:55:28

From: Johannes Berg <redacted>

There are no longer any drivers (in the tree proper, I didn't
check all the staging drivers) that take WEXT ioctls through
this API, the only remaining ones that even have ndo_do_ioctl
are using it only for private ioctls.

Therefore, we can remove this call.

Signed-off-by: Johannes Berg <redacted>
---
 net/wireless/wext-core.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/net/wireless/wext-core.c b/net/wireless/wext-core.c
index 1a4db6790e20..24ba8a99b946 100644
--- a/net/wireless/wext-core.c
+++ b/net/wireless/wext-core.c

@@ -957,9 +957,6 @@ static int wireless_process_ioctl(struct net *net, struct ifreq *ifr,
 		else if (private)
 			return private(dev, iwr, cmd, info, handler);
 	}
-	/* Old driver API : call driver ioctl handler */
-	if (dev->netdev_ops->ndo_do_ioctl)
-		return dev->netdev_ops->ndo_do_ioctl(dev, ifr, cmd);
 	return -EOPNOTSUPP;
 }

-- 
2.11.0

[PATCH 3/3] dev_ioctl: copy only the smaller struct iwreq for wext

From: Johannes Berg <johannes@sipsolutions.net>
Date: 2017-06-14 11:55:39

From: Johannes Berg <redacted>

Unfortunately, struct iwreq isn't a proper subset of struct ifreq,
but is still handled by the same code path. Robert reported that
then applications may (randomly) fault if the struct iwreq they
pass happens to land within 8 bytes of the end of a mapping (the
struct is only 32 bytes, vs. struct ifreq's 40 bytes).

To fix this, pull out the code handling wireless extension ioctls
and copy only the smaller structure in this case.

This bug goes back a long time, I tracked that it was introduced
into mainline in 2.1.15, over 20 years ago!

This fixes https://bugzilla.kernel.org/show_bug.cgi?id=195869

Reported-by: Robert O'Callahan <redacted>
Signed-off-by: Johannes Berg <redacted>
---
 include/net/wext.h       |  4 ++--
 net/core/dev_ioctl.c     | 19 ++++++++++++++++---
 net/wireless/wext-core.c |  6 +++---
 3 files changed, 21 insertions(+), 8 deletions(-)

diff --git a/include/net/wext.h b/include/net/wext.h
index 345911965dbb..454ff763eeba 100644
--- a/include/net/wext.h
+++ b/include/net/wext.h

@@ -6,7 +6,7 @@
 struct net;
 
 #ifdef CONFIG_WEXT_CORE
-int wext_handle_ioctl(struct net *net, struct ifreq *ifr, unsigned int cmd,
+int wext_handle_ioctl(struct net *net, struct iwreq *iwr, unsigned int cmd,
 		      void __user *arg);
 int compat_wext_handle_ioctl(struct net *net, unsigned int cmd,
 			     unsigned long arg);

@@ -14,7 +14,7 @@ int compat_wext_handle_ioctl(struct net *net, unsigned int cmd,
 struct iw_statistics *get_wireless_stats(struct net_device *dev);
 int call_commit_handler(struct net_device *dev);
 #else
-static inline int wext_handle_ioctl(struct net *net, struct ifreq *ifr, unsigned int cmd,
+static inline int wext_handle_ioctl(struct net *net, struct iwreq *iwr, unsigned int cmd,
 				    void __user *arg)
 {
 	return -EINVAL;

diff --git a/net/core/dev_ioctl.c b/net/core/dev_ioctl.c
index b94b1d293506..27fad31784a8 100644
--- a/net/core/dev_ioctl.c
+++ b/net/core/dev_ioctl.c

@@ -410,6 +410,22 @@ int dev_ioctl(struct net *net, unsigned int cmd, void __user *arg)
 	if (cmd == SIOCGIFNAME)
 		return dev_ifname(net, (struct ifreq __user *)arg);
 
+	/*
+	 * Take care of Wireless Extensions. Unfortunately struct iwreq
+	 * isn't a proper subset of struct ifreq (it's 8 byte shorter)
+	 * so we need to treat it specially, otherwise applications may
+	 * fault if the struct they're passing happens to land at the
+	 * end of a mapped page.
+	 */
+	if (cmd >= SIOCIWFIRST && cmd <= SIOCIWLAST) {
+		struct iwreq iwr;
+
+		if (copy_from_user(&iwr, arg, sizeof(iwr)))
+			return -EFAULT;
+
+		return wext_handle_ioctl(net, &iwr, cmd, arg);
+	}
+
 	if (copy_from_user(&ifr, arg, sizeof(struct ifreq)))
 		return -EFAULT;

@@ -559,9 +575,6 @@ int dev_ioctl(struct net *net, unsigned int cmd, void __user *arg)
 				ret = -EFAULT;
 			return ret;
 		}
-		/* Take care of Wireless Extensions */
-		if (cmd >= SIOCIWFIRST && cmd <= SIOCIWLAST)
-			return wext_handle_ioctl(net, &ifr, cmd, arg);
 		return -ENOTTY;
 	}
 }

diff --git a/net/wireless/wext-core.c b/net/wireless/wext-core.c
index 12949c8d3e5f..6cdb054484d6 100644
--- a/net/wireless/wext-core.c
+++ b/net/wireless/wext-core.c

@@ -1035,18 +1035,18 @@ static int ioctl_standard_call(struct net_device *	dev,
 }
 
 
-int wext_handle_ioctl(struct net *net, struct ifreq *ifr, unsigned int cmd,
+int wext_handle_ioctl(struct net *net, struct iwreq *iwr, unsigned int cmd,
 		      void __user *arg)
 {
 	struct iw_request_info info = { .cmd = cmd, .flags = 0 };
 	int ret;
 
-	ret = wext_ioctl_dispatch(net, (void *)ifr, cmd, &info,
+	ret = wext_ioctl_dispatch(net, iwr, cmd, &info,
 				  ioctl_standard_call,
 				  ioctl_private_call);
 	if (ret >= 0 &&
 	    IW_IS_GET(cmd) &&
-	    copy_to_user(arg, ifr, sizeof(struct iwreq)))
+	    copy_to_user(arg, iwr, sizeof(struct iwreq)))
 		return -EFAULT;
 
 	return ret;

-- 
2.11.0

[PATCH 2/3] wireless: wext: use struct iwreq earlier in the call chain

From: Johannes Berg <johannes@sipsolutions.net>
Date: 2017-06-14 11:55:57

From: Johannes Berg <redacted>

To make it clear that we never use struct ifreq, cast from it
directly in the wext entrypoint and use struct iwreq from there
on. The next patch will remove the cast again and pass the
correct struct from the beginning.

Signed-off-by: Johannes Berg <redacted>
---
 net/wireless/wext-core.c | 15 +++++++--------
 1 file changed, 7 insertions(+), 8 deletions(-)

diff --git a/net/wireless/wext-core.c b/net/wireless/wext-core.c
index 24ba8a99b946..12949c8d3e5f 100644
--- a/net/wireless/wext-core.c
+++ b/net/wireless/wext-core.c

@@ -914,13 +914,12 @@ int call_commit_handler(struct net_device *dev)
  * Main IOCTl dispatcher.
  * Check the type of IOCTL and call the appropriate wrapper...
  */
-static int wireless_process_ioctl(struct net *net, struct ifreq *ifr,
+static int wireless_process_ioctl(struct net *net, struct iwreq *iwr,
 				  unsigned int cmd,
 				  struct iw_request_info *info,
 				  wext_ioctl_func standard,
 				  wext_ioctl_func private)
 {
-	struct iwreq *iwr = (struct iwreq *) ifr;
 	struct net_device *dev;
 	iw_handler	handler;

@@ -928,7 +927,7 @@ static int wireless_process_ioctl(struct net *net, struct ifreq *ifr,
 	 * The copy_to/from_user() of ifr is also dealt with in there */
 
 	/* Make sure the device exist */
-	if ((dev = __dev_get_by_name(net, ifr->ifr_name)) == NULL)
+	if ((dev = __dev_get_by_name(net, iwr->ifr_name)) == NULL)
 		return -ENODEV;
 
 	/* A bunch of special cases, then the generic case...

@@ -974,7 +973,7 @@ static int wext_permission_check(unsigned int cmd)
 }
 
 /* entry point from dev ioctl */
-static int wext_ioctl_dispatch(struct net *net, struct ifreq *ifr,
+static int wext_ioctl_dispatch(struct net *net, struct iwreq *iwr,
 			       unsigned int cmd, struct iw_request_info *info,
 			       wext_ioctl_func standard,
 			       wext_ioctl_func private)

@@ -984,9 +983,9 @@ static int wext_ioctl_dispatch(struct net *net, struct ifreq *ifr,
 	if (ret)
 		return ret;
 
-	dev_load(net, ifr->ifr_name);
+	dev_load(net, iwr->ifr_name);
 	rtnl_lock();
-	ret = wireless_process_ioctl(net, ifr, cmd, info, standard, private);
+	ret = wireless_process_ioctl(net, iwr, cmd, info, standard, private);
 	rtnl_unlock();
 
 	return ret;

@@ -1042,7 +1041,7 @@ int wext_handle_ioctl(struct net *net, struct ifreq *ifr, unsigned int cmd,
 	struct iw_request_info info = { .cmd = cmd, .flags = 0 };
 	int ret;
 
-	ret = wext_ioctl_dispatch(net, ifr, cmd, &info,
+	ret = wext_ioctl_dispatch(net, (void *)ifr, cmd, &info,
 				  ioctl_standard_call,
 				  ioctl_private_call);
 	if (ret >= 0 &&

@@ -1104,7 +1103,7 @@ int compat_wext_handle_ioctl(struct net *net, unsigned int cmd,
 	info.cmd = cmd;
 	info.flags = IW_REQUEST_FLAG_COMPAT;
 
-	ret = wext_ioctl_dispatch(net, (struct ifreq *) &iwr, cmd, &info,
+	ret = wext_ioctl_dispatch(net, &iwr, cmd, &info,
 				  compat_standard_call,
 				  compat_private_call);

-- 
2.11.0

Re: [PATCH 0/3] wext: fix an ancient bug

From: David Miller <davem@davemloft.net>
Date: 2017-06-14 19:35:43

From: Johannes Berg <johannes@sipsolutions.net>
Date: Wed, 14 Jun 2017 13:55:20 +0200

Robert O'Callahan reported that since struct iwreq is smaller than
struct ifreq, if it happens to land at the end of a mapped section
userspace will erroneously get -EFAULT.

Bug report is https://bugzilla.kernel.org/show_bug.cgi?id=195869.

This bug is ancient, it kernel 2.1.15 was the first to contain the
bug, and was released almost exactly 20.5 years ago.

Dave, I don't know if we should even bother sending this to stable,
but if so, the third patch will apply by itself but not compile, a
smaller patch consist of only the net/core/dev_ioctl.c modifications
with a cast thrown in where the compile fails/warns due to passing
the wrong struct type.

Should I merge this through my tree?

Agreed that -stable is probably not appropriate, and yes feel free to
merge it via your tree.

20.5 years... makes me feel old :-)

`h`	back out one level
`j`	next message in thread
`k`	previous message in thread
`l`	drill in
`Esc`	close help / fold thread tree
`?`	toggle this help