[PATCH v2] net: don't unconditionally copy_from_user a struct ifreq for socket ioctls

A common implementation of isatty(3) involves calling a ioctl passing
a dummy struct argument and checking whether the syscall failed --
bionic and glibc use TCGETS (passing a struct termios), and musl uses
TIOCGWINSZ (passing a struct winsize). If the FD is a socket, we will
copy sizeof(struct ifreq) bytes of data from the argument and return
-EFAULT if that fails. The result is that the isatty implementations
may return a non-POSIX-compliant value in errno in the case where part
of the dummy struct argument is inaccessible, as both struct termios
and struct winsize are smaller than struct ifreq (at least on arm64).

Although there is usually enough stack space following the argument
on the stack that this did not present a practical problem up to now,
with MTE stack instrumentation it's more likely for the copy to fail,
as the memory following the struct may have a different tag.

Fix the problem by adding an early check for whether the ioctl is a
valid socket ioctl, and return -ENOTTY if it isn't.

Fixes: 44c02a2c3dc5 ("dev_ioctl(): move copyin/copyout to callers")
Link: https://linux-review.googlesource.com/id/I869da6cf6daabc3e4b7b82ac979683ba05e27d4d
Signed-off-by: Peter Collingbourne <redacted>
Cc: <redacted> # 4.19
---
v2:
- simplify check by using _IOC_TYPE()
- move function inline into header

 include/linux/netdevice.h | 4 ++++
 net/socket.c              | 6 +++++-
 2 files changed, 9 insertions(+), 1 deletion(-)

diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index eaf5bb008aa9..d65ce093e5a7 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h

@@ -4012,6 +4012,10 @@ int netdev_rx_handler_register(struct net_device *dev,
 void netdev_rx_handler_unregister(struct net_device *dev);
 
 bool dev_valid_name(const char *name);
+static inline bool is_socket_ioctl_cmd(unsigned int cmd)
+{
+	return _IOC_TYPE(cmd) == SOCK_IOC_TYPE;
+}
 int dev_ioctl(struct net *net, unsigned int cmd, struct ifreq *ifr,
 		bool *need_copyout);
 int dev_ifconf(struct net *net, struct ifconf *, int);

diff --git a/net/socket.c b/net/socket.c
index 0b2dad3bdf7f..8808b3617dac 100644
--- a/net/socket.c
+++ b/net/socket.c

@@ -1109,7 +1109,7 @@ static long sock_do_ioctl(struct net *net, struct socket *sock,
 		rtnl_unlock();
 		if (!err && copy_to_user(argp, &ifc, sizeof(struct ifconf)))
 			err = -EFAULT;
-	} else {
+	} else if (is_socket_ioctl_cmd(cmd)) {
 		struct ifreq ifr;
 		bool need_copyout;
 		if (copy_from_user(&ifr, argp, sizeof(struct ifreq)))

@@ -1118,6 +1118,8 @@ static long sock_do_ioctl(struct net *net, struct socket *sock,
 		if (!err && need_copyout)
 			if (copy_to_user(argp, &ifr, sizeof(struct ifreq)))
 				return -EFAULT;
+	} else {
+		err = -ENOTTY;
 	}
 	return err;
 }

@@ -3306,6 +3308,8 @@ static int compat_ifr_data_ioctl(struct net *net, unsigned int cmd,
 	struct ifreq ifreq;
 	u32 data32;
 
+	if (!is_socket_ioctl_cmd(cmd))
+		return -ENOTTY;
 	if (copy_from_user(ifreq.ifr_name, u_ifreq32->ifr_name, IFNAMSIZ))
 		return -EFAULT;
 	if (get_user(data32, &u_ifreq32->ifr_data))

-- 
2.33.0.259.gc128427fd7-goog

Hello:

This patch was applied to netdev/net.git (refs/heads/master):

On Thu, 26 Aug 2021 12:46:01 -0700 you wrote:

A common implementation of isatty(3) involves calling a ioctl passing
a dummy struct argument and checking whether the syscall failed --
bionic and glibc use TCGETS (passing a struct termios), and musl uses
TIOCGWINSZ (passing a struct winsize). If the FD is a socket, we will
copy sizeof(struct ifreq) bytes of data from the argument and return
-EFAULT if that fails. The result is that the isatty implementations
may return a non-POSIX-compliant value in errno in the case where part
of the dummy struct argument is inaccessible, as both struct termios
and struct winsize are smaller than struct ifreq (at least on arm64).

[...]

Here is the summary with links:
  - [v2] net: don't unconditionally copy_from_user a struct ifreq for socket ioctls
    https://git.kernel.org/netdev/net/c/d0efb16294d1

You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html

On Thu, 26 Aug 2021 12:46:01 -0700 Peter Collingbourne wrote:

Hi Peter, when resolving the net -> net-next merge conflict I couldn't
figure out why this chunk is needed. It seems all callers of
compat_ifr_data_ioctl() already made sure it's a socket IOCTL.
Please double check my resolution (tip of net-next) and if this is
indeed unnecessary perhaps send a cleanup? Thanks!

From: Jakub Kicinski

Sent: 31 August 2021 17:30

On Thu, 26 Aug 2021 12:46:01 -0700 Peter Collingbourne wrote:

quoted

@@ -3306,6 +3308,8 @@ static int compat_ifr_data_ioctl(struct net *net, unsigned int cmd,
 	struct ifreq ifreq;
 	u32 data32;

+	if (!is_socket_ioctl_cmd(cmd))
+		return -ENOTTY;
 	if (copy_from_user(ifreq.ifr_name, u_ifreq32->ifr_name, IFNAMSIZ))
 		return -EFAULT;
 	if (get_user(data32, &u_ifreq32->ifr_data))

Hi Peter, when resolving the net -> net-next merge conflict I couldn't
figure out why this chunk is needed. It seems all callers of
compat_ifr_data_ioctl() already made sure it's a socket IOCTL.
Please double check my resolution (tip of net-next) and if this is
indeed unnecessary perhaps send a cleanup? Thanks!

To stop the copy_from_user() faulting when the user buffer
isn't long enough.
In particular for iasatty() on arm with tagged pointers.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

On Wed, 1 Sep 2021 08:22:42 +0000 David Laight wrote:

From: Jakub Kicinski

quoted

Sent: 31 August 2021 17:30

On Thu, 26 Aug 2021 12:46:01 -0700 Peter Collingbourne wrote:  

quoted

@@ -3306,6 +3308,8 @@ static int compat_ifr_data_ioctl(struct net *net, unsigned int cmd,
 	struct ifreq ifreq;
 	u32 data32;

+	if (!is_socket_ioctl_cmd(cmd))
+		return -ENOTTY;
 	if (copy_from_user(ifreq.ifr_name, u_ifreq32->ifr_name, IFNAMSIZ))
 		return -EFAULT;
 	if (get_user(data32, &u_ifreq32->ifr_data))  

Hi Peter, when resolving the net -> net-next merge conflict I couldn't
figure out why this chunk is needed. It seems all callers of
compat_ifr_data_ioctl() already made sure it's a socket IOCTL.
Please double check my resolution (tip of net-next) and if this is
indeed unnecessary perhaps send a cleanup? Thanks!  

To stop the copy_from_user() faulting when the user buffer
isn't long enough.
In particular for iasatty() on arm with tagged pointers.

Let me rephrase. is_socket_ioctl_cmd() is always true here. There were
only two callers, both check cmd is of specific, "sockety" type.

On Wed, Sep 1, 2021 at 7:04 AM Jakub Kicinski [off-list ref] wrote:

On Wed, 1 Sep 2021 08:22:42 +0000 David Laight wrote:

quoted

From: Jakub Kicinski

quoted

Sent: 31 August 2021 17:30

On Thu, 26 Aug 2021 12:46:01 -0700 Peter Collingbourne wrote:

quoted

@@ -3306,6 +3308,8 @@ static int compat_ifr_data_ioctl(struct net *net, unsigned int cmd,
  struct ifreq ifreq;
  u32 data32;

+ if (!is_socket_ioctl_cmd(cmd))
+         return -ENOTTY;
  if (copy_from_user(ifreq.ifr_name, u_ifreq32->ifr_name, IFNAMSIZ))
          return -EFAULT;
  if (get_user(data32, &u_ifreq32->ifr_data))

Hi Peter, when resolving the net -> net-next merge conflict I couldn't
figure out why this chunk is needed. It seems all callers of
compat_ifr_data_ioctl() already made sure it's a socket IOCTL.
Please double check my resolution (tip of net-next) and if this is
indeed unnecessary perhaps send a cleanup? Thanks!

To stop the copy_from_user() faulting when the user buffer
isn't long enough.
In particular for iasatty() on arm with tagged pointers.

Let me rephrase. is_socket_ioctl_cmd() is always true here. There were
only two callers, both check cmd is of specific, "sockety" type.

I see, it looks like we don't need the check on the compat path then.

I can send a followup to clean this up but given that I got a comment
from another reviewer saying that we should try to make the native and
compat paths as similar as possible, maybe it isn't too bad to leave
things as is?

Peter

On Wed, 1 Sep 2021 11:01:32 -0700 Peter Collingbourne wrote:

quoted

quoted

To stop the copy_from_user() faulting when the user buffer
isn't long enough.
In particular for iasatty() on arm with tagged pointers.  

Let me rephrase. is_socket_ioctl_cmd() is always true here. There were
only two callers, both check cmd is of specific, "sockety" type.  

I see, it looks like we don't need the check on the compat path then.

I can send a followup to clean this up but given that I got a comment
from another reviewer saying that we should try to make the native and
compat paths as similar as possible, maybe it isn't too bad to leave
things as is?

I have a weak preference to get rid of it, the code is a little
complex and extra dead code makes it harder to follow, but up to you.

IMO the "right place" for the check is:

static long sock_ioctl(struct file *file, unsigned cmd, unsigned long arg)
[...]
		default:
			/* --> here <-- */
			err = sock_do_ioctl(net, sock, cmd, arg);
			break;

Since that's the point where we take all the remaining cmd values and
call a function which assumes struct ifreq.

Compat code does not have a default statement.

But as I said no big deal, feel free to ignore.