linux-kernel - Re: [PATCH v2] tty: n_gsm: restrict tty devices to attach

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: <563ec0ed-a851-450b-aed6-986f6ea324ca@I-love.SAKURA.ne.jp>
Date: Wed, 24 Apr 2024 00:26:53 +0900
From: Tetsuo Handa <penguin-kernel@...ove.SAKURA.ne.jp>
To: Linus Torvalds <torvalds@...ux-foundation.org>
Cc: Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Jiri Slaby <jirislaby@...nel.org>,
        Andrew Morton
 <akpm@...ux-foundation.org>,
        "Starke, Daniel" <daniel.starke@...mens.com>,
        LKML <linux-kernel@...r.kernel.org>,
        linux-security-module <linux-security-module@...r.kernel.org>
Subject: Re: [PATCH v2] tty: n_gsm: restrict tty devices to attach

On 2024/04/22 1:04, Linus Torvalds wrote:
>> Now, your 'struct tty_operations' flag saying 'my ->write() function is OK with
>> atomic context' is expected to be set to all drivers.
> 
> I'm not convinced. The only thing I know is that the comment in
> question is wrong, and has been wrong for over a decade (and honestly,
> probably pretty much forever).
> 
> So how confident are we that other tty write functions are ok?
> 
> Also, since you think that only con_write() has a problem, why the
> heck are you then testing for ptys etc? From a quick check, the
> pty->ops->write() function is fine.

I tried to make deny-listing as close as allow-listing. But it seems that
ipw_write() in drivers/tty/ipwireless/tty.c (e.g. /dev/ttyIPWp0 ) does
sleep as well as con_write() in drivers/tty/vt/vt.c .

I couldn't judge serial_write() in drivers/usb/serial/usb-serial.c (e.g.
/dev/ttyUSB0 ). But since device number for /dev/ttyIPWp0 is assigned
dynamically, we can't rely on major/minor for detecting /dev/ttyIPWp0 from
gsmld_open() side.

That is, we need to handle this problem from "struct tty_operations" side
(like I initially tried).



On 2024/04/22 2:18, Linus Torvalds wrote:
> On Sun, 21 Apr 2024 at 09:04, Linus Torvalds
> <torvalds@...ux-foundation.org> wrote:
>>
>> The only option is to *mark* the ones that are atomic. Which was my suggestion.

Since majority of "struct tty_operations"->write() seems to be atomic,
I prefer updating only ones which cannot be atomic.

> 
> Actually, another option would be to just return an error at 'set_ldisc()' time.
> 
> Sadly, the actual "tty->ops->set_ldisc()" function not only returns
> 'void' (easy enough to change - there aren't that many of them), but
> it's called too late after the old ldisc has already been dropped.
> It's basically a "inform tty about new ldisc" and is not useful for a
> "is this ok"?
> 
> But we could trivially add a "ldisc_ok()" function, and have the vt
> driver say "I only accept N_TTY".
> 
> Something like this ENTIRELY UNTESTED patch.
> 
> Again - this is untested, and maybe there are other tty drivers that
> have issues with the stranger line disciplines, but this at least
> seems simple and fairly easy to explain why we do what we do..

This patch works for me. You can propose a formal patch.