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

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

[Peter Collingbourne]

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 <pcc@google.com>
Cc: <stable@vger.kernel.org> # 4.19
---
 include/linux/netdevice.h |  1 +
 net/core/dev_ioctl.c      | 64 ++++++++++++++++++++++++++++++++-------
 net/socket.c              |  6 +++-
 3 files changed, 59 insertions(+), 12 deletions(-)

diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index eaf5bb008aa9..481b90ef0d32 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -4012,6 +4012,7 @@ 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);
+bool is_dev_ioctl_cmd(unsigned int cmd);
 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/core/dev_ioctl.c b/net/core/dev_ioctl.c
index 478d032f34ac..ac807fc64da1 100644
--- a/net/core/dev_ioctl.c
+++ b/net/core/dev_ioctl.c
@@ -368,6 +368,54 @@ void dev_load(struct net *net, const char *name)
 }
 EXPORT_SYMBOL(dev_load);
 
+bool is_dev_ioctl_cmd(unsigned int cmd)
+{
+	switch (cmd) {
+	case SIOCGIFNAME:
+	case SIOCGIFHWADDR:
+	case SIOCGIFFLAGS:
+	case SIOCGIFMETRIC:
+	case SIOCGIFMTU:
+	case SIOCGIFSLAVE:
+	case SIOCGIFMAP:
+	case SIOCGIFINDEX:
+	case SIOCGIFTXQLEN:
+	case SIOCETHTOOL:
+	case SIOCGMIIPHY:
+	case SIOCGMIIREG:
+	case SIOCSIFNAME:
+	case SIOCSIFMAP:
+	case SIOCSIFTXQLEN:
+	case SIOCSIFFLAGS:
+	case SIOCSIFMETRIC:
+	case SIOCSIFMTU:
+	case SIOCSIFHWADDR:
+	case SIOCSIFSLAVE:
+	case SIOCADDMULTI:
+	case SIOCDELMULTI:
+	case SIOCSIFHWBROADCAST:
+	case SIOCSMIIREG:
+	case SIOCBONDENSLAVE:
+	case SIOCBONDRELEASE:
+	case SIOCBONDSETHWADDR:
+	case SIOCBONDCHANGEACTIVE:
+	case SIOCBRADDIF:
+	case SIOCBRDELIF:
+	case SIOCSHWTSTAMP:
+	case SIOCBONDSLAVEINFOQUERY:
+	case SIOCBONDINFOQUERY:
+	case SIOCGIFMEM:
+	case SIOCSIFMEM:
+	case SIOCSIFLINK:
+	case SIOCWANDEV:
+	case SIOCGHWTSTAMP:
+		return true;
+
+	default:
+		return cmd >= SIOCDEVPRIVATE && cmd <= SIOCDEVPRIVATE + 15;
+	}
+}
+
 /*
  *	This function handles all "interface"-type I/O control requests. The actual
  *	'doing' part of this is dev_ifsioc above.
@@ -521,16 +569,10 @@ int dev_ioctl(struct net *net, unsigned int cmd, struct ifreq *ifr, bool *need_c
 	 *	Unknown or private ioctl.
 	 */
 	default:
-		if (cmd == SIOCWANDEV ||
-		    cmd == SIOCGHWTSTAMP ||
-		    (cmd >= SIOCDEVPRIVATE &&
-		     cmd <= SIOCDEVPRIVATE + 15)) {
-			dev_load(net, ifr->ifr_name);
-			rtnl_lock();
-			ret = dev_ifsioc(net, ifr, cmd);
-			rtnl_unlock();
-			return ret;
-		}
-		return -ENOTTY;
+		dev_load(net, ifr->ifr_name);
+		rtnl_lock();
+		ret = dev_ifsioc(net, ifr, cmd);
+		rtnl_unlock();
+		return ret;
 	}
 }
diff --git a/net/socket.c b/net/socket.c
index 0b2dad3bdf7f..e58886b1882c 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_dev_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_dev_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.rc2.250.ged5fa647cd-goog

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

[Greg KH]

On Wed, Aug 25, 2021 at 06:27:22PM -0700, Peter Collingbourne 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).
> 
> 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 <pcc@google.com>
> Cc: <stable@vger.kernel.org> # 4.19
> ---
>  include/linux/netdevice.h |  1 +
>  net/core/dev_ioctl.c      | 64 ++++++++++++++++++++++++++++++++-------
>  net/socket.c              |  6 +++-
>  3 files changed, 59 insertions(+), 12 deletions(-)
> 
> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> index eaf5bb008aa9..481b90ef0d32 100644
> --- a/include/linux/netdevice.h
> +++ b/include/linux/netdevice.h
> @@ -4012,6 +4012,7 @@ 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);
> +bool is_dev_ioctl_cmd(unsigned int cmd);

"is_socket_ioctl_cmd()" might be a better global name here.

thanks,

greg k-h

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

[David Laight]

From: Peter Collingbourne
> Sent: 26 August 2021 02:27
> 
> 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.
..
> +bool is_dev_ioctl_cmd(unsigned int cmd)
> +{
> +	switch (cmd) {
> +	case SIOCGIFNAME:
> +	case SIOCGIFHWADDR:
> +	case SIOCGIFFLAGS:
> +	case SIOCGIFMETRIC:
> +	case SIOCGIFMTU:
> +	case SIOCGIFSLAVE:
> +	case SIOCGIFMAP:
> +	case SIOCGIFINDEX:
> +	case SIOCGIFTXQLEN:
> +	case SIOCETHTOOL:
> +	case SIOCGMIIPHY:
> +	case SIOCGMIIREG:
> +	case SIOCSIFNAME:
> +	case SIOCSIFMAP:
> +	case SIOCSIFTXQLEN:
> +	case SIOCSIFFLAGS:
> +	case SIOCSIFMETRIC:
> +	case SIOCSIFMTU:
> +	case SIOCSIFHWADDR:
> +	case SIOCSIFSLAVE:
> +	case SIOCADDMULTI:
> +	case SIOCDELMULTI:
> +	case SIOCSIFHWBROADCAST:
> +	case SIOCSMIIREG:
> +	case SIOCBONDENSLAVE:
> +	case SIOCBONDRELEASE:
> +	case SIOCBONDSETHWADDR:
> +	case SIOCBONDCHANGEACTIVE:
> +	case SIOCBRADDIF:
> +	case SIOCBRDELIF:
> +	case SIOCSHWTSTAMP:
> +	case SIOCBONDSLAVEINFOQUERY:
> +	case SIOCBONDINFOQUERY:
> +	case SIOCGIFMEM:
> +	case SIOCSIFMEM:
> +	case SIOCSIFLINK:
> +	case SIOCWANDEV:
> +	case SIOCGHWTSTAMP:
> +		return true;

That is horrid.
Can't you at least use _IOC_TYPE() to check for socket ioctls.
Clearly it can succeed for 'random' driver ioctls, but will fail
for the tty ones.

The other sane thing is to check _IOC_SIZE().
Since all the SIOCxxxx have a correct _IOC_SIZE() that can be
used to check the user copy length.
(Unlike socket options the correct length is always supplied.

	David

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

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

[Arnd Bergmann]

On Thu, Aug 26, 2021 at 3:28 AM Peter Collingbourne <pcc@google.com> wrote:

> -       } else {
> +       } else if (is_dev_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_dev_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))

This adds yet another long switch() statement into the socket ioctl
case, when there
is already one in compat_sock_ioctl_trans(), one in dev_ifsioc() and one in
dev_ioctl(), all with roughly the same set of ioctl command codes. If
any of them
are called frequently, that makes it all even slower, so I wonder if
there should
be a larger rework altogether. Maybe something based on a single lookup table
that we search through directly from sock_ioctl()/compat_sock_ioctl() to deal
with the differences in handling (ifreq based, compat handler, proto_ops
override, dev_load, rtnl_lock, rcu_read_lock, CAP_NET_ADMIN, copyout, ...).

You are also adding the checks into different places for native and compat
mode, which makes them diverge more when we should be trying to
make them more common.

I think based on my recent changes, some other simplifications are possible,
based on how the compat path already enumerates all the dev ioctls.

        Arnd

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

[Peter Collingbourne]

On Wed, Aug 25, 2021 at 11:39 PM Greg KH <gregkh@linuxfoundation.org> wrote:
>
> On Wed, Aug 25, 2021 at 06:27:22PM -0700, Peter Collingbourne 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).
> >
> > 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 <pcc@google.com>
> > Cc: <stable@vger.kernel.org> # 4.19
> > ---
> >  include/linux/netdevice.h |  1 +
> >  net/core/dev_ioctl.c      | 64 ++++++++++++++++++++++++++++++++-------
> >  net/socket.c              |  6 +++-
> >  3 files changed, 59 insertions(+), 12 deletions(-)
> >
> > diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> > index eaf5bb008aa9..481b90ef0d32 100644
> > --- a/include/linux/netdevice.h
> > +++ b/include/linux/netdevice.h
> > @@ -4012,6 +4012,7 @@ 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);
> > +bool is_dev_ioctl_cmd(unsigned int cmd);
>
> "is_socket_ioctl_cmd()" might be a better global name here.

SGTM, done in v2.

Peter

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

[Peter Collingbourne]

On Thu, Aug 26, 2021 at 1:12 AM David Laight <David.Laight@aculab.com> wrote:
>
> From: Peter Collingbourne
> > Sent: 26 August 2021 02:27
> >
> > 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.
> ..
> > +bool is_dev_ioctl_cmd(unsigned int cmd)
> > +{
> > +     switch (cmd) {
> > +     case SIOCGIFNAME:
> > +     case SIOCGIFHWADDR:
> > +     case SIOCGIFFLAGS:
> > +     case SIOCGIFMETRIC:
> > +     case SIOCGIFMTU:
> > +     case SIOCGIFSLAVE:
> > +     case SIOCGIFMAP:
> > +     case SIOCGIFINDEX:
> > +     case SIOCGIFTXQLEN:
> > +     case SIOCETHTOOL:
> > +     case SIOCGMIIPHY:
> > +     case SIOCGMIIREG:
> > +     case SIOCSIFNAME:
> > +     case SIOCSIFMAP:
> > +     case SIOCSIFTXQLEN:
> > +     case SIOCSIFFLAGS:
> > +     case SIOCSIFMETRIC:
> > +     case SIOCSIFMTU:
> > +     case SIOCSIFHWADDR:
> > +     case SIOCSIFSLAVE:
> > +     case SIOCADDMULTI:
> > +     case SIOCDELMULTI:
> > +     case SIOCSIFHWBROADCAST:
> > +     case SIOCSMIIREG:
> > +     case SIOCBONDENSLAVE:
> > +     case SIOCBONDRELEASE:
> > +     case SIOCBONDSETHWADDR:
> > +     case SIOCBONDCHANGEACTIVE:
> > +     case SIOCBRADDIF:
> > +     case SIOCBRDELIF:
> > +     case SIOCSHWTSTAMP:
> > +     case SIOCBONDSLAVEINFOQUERY:
> > +     case SIOCBONDINFOQUERY:
> > +     case SIOCGIFMEM:
> > +     case SIOCSIFMEM:
> > +     case SIOCSIFLINK:
> > +     case SIOCWANDEV:
> > +     case SIOCGHWTSTAMP:
> > +             return true;
>
> That is horrid.
> Can't you at least use _IOC_TYPE() to check for socket ioctls.
> Clearly it can succeed for 'random' driver ioctls, but will fail
> for the tty ones.

Yes, that works, since all of the ioctls listed above are in the range
where the _IOC_TYPE() check would succeed. It now also makes sense to
move the check inline into the header. I've done all of that in v2.

> The other sane thing is to check _IOC_SIZE().
> Since all the SIOCxxxx have a correct _IOC_SIZE() that can be
> used to check the user copy length.
> (Unlike socket options the correct length is always supplied.

FWIW, it doesn't look like any of them have the _IOC_SIZE() bits set,
so that won't work. _IOC_TYPE() seems better anyway.

Peter

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

[Peter Collingbourne]

On Thu, Aug 26, 2021 at 1:59 AM Arnd Bergmann <arnd@kernel.org> wrote:
>
> On Thu, Aug 26, 2021 at 3:28 AM Peter Collingbourne <pcc@google.com> wrote:
>
> > -       } else {
> > +       } else if (is_dev_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_dev_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))
>
> This adds yet another long switch() statement into the socket ioctl
> case, when there
> is already one in compat_sock_ioctl_trans(), one in dev_ifsioc() and one in
> dev_ioctl(), all with roughly the same set of ioctl command codes. If

I think that David's suggestion of using _IOC_TYPE() should be enough
to address this for now.

> any of them
> are called frequently, that makes it all even slower, so I wonder if
> there should
> be a larger rework altogether. Maybe something based on a single lookup table
> that we search through directly from sock_ioctl()/compat_sock_ioctl() to deal
> with the differences in handling (ifreq based, compat handler, proto_ops
> override, dev_load, rtnl_lock, rcu_read_lock, CAP_NET_ADMIN, copyout, ...).
>
> You are also adding the checks into different places for native and compat
> mode, which makes them diverge more when we should be trying to
> make them more common.
>
> I think based on my recent changes, some other simplifications are possible,
> based on how the compat path already enumerates all the dev ioctls.

I think we should leave that for a followup if still necessary.

Peter

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

[David Laight]

From: Peter Collingbourne
> Sent: 26 August 2021 20:46
...
> > The other sane thing is to check _IOC_SIZE().
> > Since all the SIOCxxxx have a correct _IOC_SIZE() that can be
> > used to check the user copy length.
> > (Unlike socket options the correct length is always supplied.
> 
> FWIW, it doesn't look like any of them have the _IOC_SIZE() bits set,
> so that won't work. _IOC_TYPE() seems better anyway.

Linus must have stolen those definitions from SVSV not one of the BSDs.
The BSD's started using the high 16 bits when they moved to 32bit.

Something I've written kernel code for required those bits be set
and would then do the user copies in the syscall entry paths.
It won't be SYSV because I used 3 character 'type' fields on that.
Windows does do the copies - but is entirely 'not quite' different.
So it must have been NetBDSD.

	David

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