linux-kernel - Re: [PATCH v2] tty: ttynull: implement console write

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: <Y+0EnQ/Ujtq+nEFf@alley>
Date:   Wed, 15 Feb 2023 17:13:17 +0100
From:   Petr Mladek <pmladek@...e.com>
To:     John Ogness <john.ogness@...utronix.de>
Cc:     Vincent Whitchurch <vincent.whitchurch@...s.com>,
        Michael Thalmeier <michael.thalmeier@...e.at>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Jiri Slaby <jirislaby@...nel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2] tty: ttynull: implement console write

On Wed 2023-02-15 16:24:23, John Ogness wrote:
> On 2023-02-15, Petr Mladek <pmladek@...e.com> wrote:
> > That said, the current code is error-prone. The check for non-NULL
> > con->write is too far from the caller.
> 
> con->write is supposed to be immutable after registration, so having the
> check "far from the caller" is not a real danger.
> 
> console_emit_next_record() is the toplevel function responsible for
> printing on a particular console so I think it is appropriate that the
> check is made when determining if this function should be called. I also
> think console_is_usable() is the proper place for the NULL check to
> reside since that is the function that determines if printing is
> allowed.

I agree that the current code is not that bad. But still, the call
chain is:

  + console_flush_all()
    + console_emit_next_record()
      + call_console_driver()
	+ con->write()

I could imagine another code that would call call_console_driver()
or console_emit_next_record() directly. We might just hope that
it would check console_is_usable() or con->write pointer before.
I consider this error-prone.

Also, as you say, con->write is immutable. All real consoles have it
defined. It does not make sense to check it again and again.
I would leave console_is_usable() for checks that might really
change at runtime.

> > I would prefer to make it more safe. For example, I would prevent
> > registration of consoles without con->write callback in the first
> > place. It would require, to implement the empty write() callback
> > for ttynull console as done by this patch.
> 
> I would prefer that we do not start encouraging dummy implementations.
> If you insist on con->write having _some_ value other than NULL, then we
> could define some macro with a special value (CONSOLE_NO_WRITE). But
> then we have to check that value. IMHO, allowing NULL is not an issue.

ttynull is really special. It is a dummy driver and dummy callbacks
are perfectly fine. IMHO, one dummy driver is enough. Ideally,
the generic printk code should not need any special hacks to
handle it.

IMHO, normal console drivers would be useless without write()
callback. It sounds perfectly fine to reject useless console
drivers at all. And it sounds like a reasonable check
in register_console() that would reject bad console drivers.

Best Regards,
Petr