linux-kernel - Re: [PATCHES] tty ioctls cleanups, compat and not only

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20180914151011.GZ19965@ZenIV.linux.org.uk>
Date:   Fri, 14 Sep 2018 16:10:11 +0100
From:   Al Viro <viro@...IV.linux.org.uk>
To:     Arnd Bergmann <arnd@...db.de>
Cc:     David Miller <davem@...emloft.net>,
        gregkh <gregkh@...uxfoundation.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [PATCHES] tty ioctls cleanups, compat and not only

On Fri, Sep 14, 2018 at 10:21:53AM +0200, Arnd Bergmann wrote:

> This does sound very appealing, but there is a small downside:
> The difference between ".compat_ioctl = NULL" and
> ".compat_ioctl=native_ioctl" is now very subtle, and I wouldn't
> necessarily expect casual readers to understand that.

???

One is "all non-generics take pointers to wordsize-neutral objects",
another "all non-generics take integers".

That solution certainly needs to be documented more than just in commit
message, though; IMO the method descriptions next to declaration are the
best place for that.  Will update...

> If we go with my file_operations patch for generic_compat_ioctl_ptrarg
> and add generic_compat_ioctl_intarg, we can do the same thing here
> with ldisc_compat_ioctl_ptrarg/ldisc_compat_ioctl_intarg to make it
> a little more consistent with fops and self-documenting.

No, we can't - ldisc ->ioctl() (or ->compat_ioctl()) doesn't get ldisc
in arguments.  Besides, indirect calls are costly these days...

> +        if (retval == -ENOIOCTLCMD && _IOC_TYPE(cmd) == 'T') {
> +                 retval = tty_ioctl(file, cmd, (unsigned long)compat_ptr(arg));
> +                 WARN_ON_ONCE(retval != -ENOIOCTLCMD && retval != -ENOTTY);
> +        }
> 
> Seeing that you list every single 'T' command in tty_compat_ioctl()
> that we handle in the native case, that WARN_ON_ONCE should not
> trigger for any input, but it would catch (and warn about) any of those
> that might get added in the future to the native code path without the
> compat entry.

Anything adding new ioctls needs careful review anyway.  And blind bets upon
that stuff taking compat pointer are, AFAICS, completely unfounded.