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: <CAK8P3a2LpC6sw5KQ_6kcXKNfgUXkjsxbFkD30kdMgQjurGgy9Q@mail.gmail.com>
Date:   Thu, 13 Sep 2018 13:19:42 +0200
From:   Arnd Bergmann <arnd@...db.de>
To:     Al Viro <viro@...iv.linux.org.uk>
Cc:     gregkh <gregkh@...uxfoundation.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [PATCHES] tty ioctls cleanups, compat and not only

On Thu, Sep 13, 2018 at 4:31 AM Al Viro <viro@...iv.linux.org.uk> wrote:
>
>         See vfs.git#work.tty-ioctl.  Completely untested, should seriously
> clean the things up wrt compat.  Remaining problems (aside of the bugs
> introduced in it, of course):
>         * TIOCSERGSTRUCT must die; it's present only in amiserial and it's
> _vile_; look at what it copies out and weep.
>         * synclink_gt has proper compat handling for its private ioctls;
> other synclink drivers (with the same ioctls) do not.
>         * dgnc definitely has buggered ioctls - structs full of longs are
> bloody bad idea for passing around.  It's in staging, and I'd say that it
> needs the userland ABI fixed.
>         * cyclades, rocket, moxa and mxser probably have non-trivial
> problems with their private ioctls; I hadn't looked into those.
>         * n_gsm needs ->compat_ioctl(); easy to do, I just hadn't done it
> yet.
>         * ipwireless might or might not need compat_ioctl (PPP stuff in it);
> not sure.
>         * ldisc private ioctls need more work.  Hadn't gone there yet.
> Generic ioctls should be fine - they never reach ->compat_ioctl() with this
> series.

I looked at all the new patches, looks all good to me. When I did my
(incomplete) analysis, I found a couple more things that could be
included in the series if we want to:

* TIOCSERGWILD/TIOCSERSWILD are obsolete and never do
   anything useful, but the return code is inconsistent: ENOTTY
   in compat mode vs 0 in uart_ioctl, plus a printk for amiserial.
   If we want to keep it around rather than deleting it completely,
   it should be marked as compatible.

* PPPIOCGCHAN/PPPIOCGUNIT are implemented by multiple
   ldisc variants, marking them as compatible would save us from
   implementing a comp_ioctl method for each one separately.
   These are also used on some things that are not ttys though,
   so we can't remove them from fs/compat_ioctl.c yet.

* FIONREAD/TIOCINQ and TIOCOUTQ also appear multiple
   times and could be added to that list but not removed from
   fs/compat_ioctl.c (I think the other users are all in sockets, so
   adding them in both might be sufficient)

* SIOCGIFNAME, SIOCGIFENCAP, SIOCSIFENCAP,
  SIOCSIFHWADDR, SIOCSKEEPALIVE, SIOCGKEEPALIVE,
  SIOCSOUTFILL,  and SIOCGOUTFILL  are in the tty_ioctl
  functions for multiple protocol handlers, comparable to
  the PPP ones.

>         There's some overlap with patches posted earlier - e.g. I'm fairly
> sure that keyboard ioctl "remove from fs/compat_ioctl.c" part duplicates
> one of yours; I'll be glad to drop and replace such duplicates with already
> posted variants.

Ok, don't worry about the order if you decide to apply yours first,
you can just drop mine.

        Arnd