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)
