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

On Thu, Aug 26, 2021 at 1:12 AM David Laight [off-list ref] wrote:

From: Peter Collingbourne

quoted

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.

..

quoted

+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