Re: [PATCH v12 1/9] security: Introduce ENOFILEOPS return value for IOCTL hooks

["Arnd Bergmann" <arnd@arndb.de>]

On Mon, Mar 25, 2024, at 14:39, Günther Noack wrote:
> If security_file_ioctl or security_file_ioctl_compat return
> ENOFILEOPS, the IOCTL logic in fs/ioctl.c will permit the given IOCTL
> command, but only as long as the IOCTL command is implemented directly
> in fs/ioctl.c and does not use the f_ops->unhandled_ioctl or
> f_ops->compat_ioctl operations, which are defined by the given file.
>
> The possible return values for security_file_ioctl and
> security_file_ioctl_compat are now:
>
>  * 0 - to permit the IOCTL
>  * ENOFILEOPS - to permit the IOCTL, but forbid it if it needs to fall
>    back to the file implementation.
>  * any other error - to forbid the IOCTL and return that error
>
> This is an alternative to the previously discussed approaches [1] and [2],
> and implements the proposal from [3].

Thanks for trying it out, I think this is a good solution
and I like how the code turned out.

One small thing that I believe needs some extra changes:

> @@ -967,6 +977,11 @@ COMPAT_SYSCALL_DEFINE3(ioctl, unsigned int, fd, 
> unsigned int, cmd,
>  		if (error != -ENOIOCTLCMD)
>  			break;
> 
> +		if (!use_file_ops) {
> +			error = -EACCES;
> +			break;
> +		}
> +
>  		if (f.file->f_op->compat_ioctl)
>  			error = f.file->f_op->compat_ioctl(f.file, cmd, arg);
>  		if (error == -ENOIOCTLCMD)

The compat FIONREAD handler now ends up calling ->compat_ioctl()
where it used to call ->ioctl(). I think this means we need to
audit every driver that implements its own
FIONREAD/SIOCINQ/TIOCINQ to make sure it has a working compat
implementation.

I have done one pass through all such drivers and think the
change below should be sufficient for all of them, but please
have another look. Feel free to fold this change into your
patch. The pipe.c change also fixes an existing bug with 
IOC_WATCH_QUEUE_SET_SIZE/IOC_WATCH_QUEUE_SET_FILTER, so that
may need to be a separate patch and get backported.

    Arnd

Signed-off-by: Arnd Bergmann <arnd@arndb.de>

diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
index 407b0d87b7c1..2e5b495a5606 100644
--- a/drivers/tty/tty_io.c
+++ b/drivers/tty/tty_io.c
@@ -2891,6 +2891,7 @@ static long tty_compat_ioctl(struct file *file, unsigned int cmd,
        int retval = -ENOIOCTLCMD;
 
        switch (cmd) {
+       case TIOCINQ:
        case TIOCOUTQ:
        case TIOCSTI:
        case TIOCGWINSZ:
diff --git a/fs/pipe.c b/fs/pipe.c
index 50c8a8596b52..a6ebb351ea4b 100644
--- a/fs/pipe.c
+++ b/fs/pipe.c
@@ -652,6 +652,14 @@ static long pipe_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
        }
 }
 
+static long pipe_compat_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
+{
+       if (cmd == IOC_WATCH_QUEUE_SET_SIZE)
+               return pipe_ioctl(filp, cmd, arg);
+
+       return compat_ptr_ioctl(filp, cmd, arg);
+}
+
 /* No kernel lock held - fine */
 static __poll_t
 pipe_poll(struct file *filp, poll_table *wait)
@@ -1234,6 +1242,7 @@ const struct file_operations pipefifo_fops = {
        .write_iter     = pipe_write,
        .poll           = pipe_poll,
        .unlocked_ioctl = pipe_ioctl,
+       .compat_ioctl   = pipe_compat_ioctl,
        .release        = pipe_release,
        .fasync         = pipe_fasync,
        .splice_write   = iter_file_splice_write,
diff --git a/net/socket.c b/net/socket.c
index e5f3af49a8b6..bb4fa51fe4ca 100644
--- a/net/socket.c
+++ b/net/socket.c
@@ -3496,6 +3496,7 @@ static int compat_sock_ioctl_trans(struct file *file, struct socket *sock,
        case SIOCSARP:
        case SIOCGARP:
        case SIOCDARP:
+       case SIOCINQ:
        case SIOCOUTQ:
        case SIOCOUTQNSD:
        case SIOCATMARK: