[PATCH v2] tty: ttynull: implement console write

[PATCH v2] tty: ttynull: implement console write

[Michael Thalmeier]

Since commit a699449bb13b ("printk: refactor and rework printing logic"),
con->write is called without checking if it is implemented in the console
driver. This does make some sense, because for all "normal" consoles it
is vital to have a write function.
As the ttynull console driver does not need any console output the write
function was not implemented. This caused a "unable to handle kernel NULL
pointer dereference" when the write function is called now.

To fix this issue, implement an empty write function.

Fixes: a699449bb13b ("printk: refactor and rework printing logic")
Cc: stable@vger.kernel.org
Signed-off-by: Michael Thalmeier <michael.thalmeier@hale.at>
---
Changes in v2:
- Reference correct commit SHA-1 ID
- Add Fixes tag
- Link to v1: https://lore.kernel.org/all/20230214102317.382769-1-michael.thalmeier@hale.at

---
 drivers/tty/ttynull.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/tty/ttynull.c b/drivers/tty/ttynull.c
index 1d4438472442..6e9323544a7d 100644
--- a/drivers/tty/ttynull.c
+++ b/drivers/tty/ttynull.c
@@ -40,6 +40,12 @@ static unsigned int ttynull_write_room(struct tty_struct *tty)
 	return 65536;
 }
 
+
+static void ttynull_console_write(struct console *co, const char *buf,
+				  unsigned count)
+{
+}
+
 static const struct tty_operations ttynull_ops = {
 	.open = ttynull_open,
 	.close = ttynull_close,
@@ -56,6 +62,7 @@ static struct tty_driver *ttynull_device(struct console *c, int *index)
 
 static struct console ttynull_console = {
 	.name = "ttynull",
+	.write = ttynull_console_write,
 	.device = ttynull_device,
 };
 
-- 
2.39.1

Re: [PATCH v2] tty: ttynull: implement console write

[Vincent Whitchurch]

+ Cc: John, Petr

On Tue, Feb 14, 2023 at 12:59:21PM +0100, Michael Thalmeier wrote:
> Since commit a699449bb13b ("printk: refactor and rework printing logic"),
> con->write is called without checking if it is implemented in the console
> driver. This does make some sense, because for all "normal" consoles it
> is vital to have a write function.
> As the ttynull console driver does not need any console output the write
> function was not implemented. This caused a "unable to handle kernel NULL
> pointer dereference" when the write function is called now.
> 
> To fix this issue, implement an empty write function.
> 
> Fixes: a699449bb13b ("printk: refactor and rework printing logic")
> Cc: stable@vger.kernel.org
> Signed-off-by: Michael Thalmeier <michael.thalmeier@hale.at>

Looking at the referenced commit, the only place I see it calling
con->write() is from call_console_driver(), which is in turn only called
from console_emit_next_record().  And console_flush_all(), the only
caller of console_emit_next_record(), checks that con->write is not NULL
using console_is_usable() before calling console_emit_next_record().

What am I missing?  Which code path in the referenced commit calls
con->write without checking for NULL?

Re: [PATCH v2] tty: ttynull: implement console write

[Petr Mladek]

On Wed 2023-02-15 12:37:36, Vincent Whitchurch wrote:
> + Cc: John, Petr
> 
> On Tue, Feb 14, 2023 at 12:59:21PM +0100, Michael Thalmeier wrote:
> > Since commit a699449bb13b ("printk: refactor and rework printing logic"),
> > con->write is called without checking if it is implemented in the console
> > driver. This does make some sense, because for all "normal" consoles it
> > is vital to have a write function.
> > As the ttynull console driver does not need any console output the write
> > function was not implemented. This caused a "unable to handle kernel NULL
> > pointer dereference" when the write function is called now.
> > 
> > To fix this issue, implement an empty write function.
> > 
> > Fixes: a699449bb13b ("printk: refactor and rework printing logic")
> > Cc: stable@vger.kernel.org
> > Signed-off-by: Michael Thalmeier <michael.thalmeier@hale.at>
> 
> Looking at the referenced commit, the only place I see it calling
> con->write() is from call_console_driver(), which is in turn only called
> from console_emit_next_record().  And console_flush_all(), the only
> caller of console_emit_next_record(), checks that con->write is not NULL
> using console_is_usable() before calling console_emit_next_record().

I see the same. The refactoring moved the check of con->write from
call_console_driver() to console_is_usable(). It detects the NULL
pointer earlier in console_flush_all()...

> What am I missing?  Which code path in the referenced commit calls
> con->write without checking for NULL?

Vincent, could you please provide log with the backtrace?

I wonder if the problem is in the RT-patchset where
console_emit_next_record() is called also from the printk kthread.

That said, the current code is error-prone. The check for non-NULL
con->write is too far from the caller.

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.

Anyway, I would like to understand if the "unable to handle kernel NULL
pointer dereference" is a real problem in the current implementation.

Best Regards,
Petr

Re: [PATCH v2] tty: ttynull: implement console write

[John Ogness]

On 2023-02-15, Petr Mladek <pmladek@suse.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 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.

John Ogness

Re: [PATCH v2] tty: ttynull: implement console write

[Petr Mladek]

On Wed 2023-02-15 16:24:23, John Ogness wrote:
> On 2023-02-15, Petr Mladek <pmladek@suse.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

Re: [PATCH v2] tty: ttynull: implement console write

[Michael Thalmeier]

Hi Petr,

----- On 15 Feb, 2023, at 15:33, Petr Mladek pmladek@suse.com wrote:
> On Wed 2023-02-15 12:37:36, Vincent Whitchurch wrote:
>> + Cc: John, Petr
>> 
>> On Tue, Feb 14, 2023 at 12:59:21PM +0100, Michael Thalmeier wrote:
>> > Since commit a699449bb13b ("printk: refactor and rework printing logic"),
>> > con->write is called without checking if it is implemented in the console
>> > driver. This does make some sense, because for all "normal" consoles it
>> > is vital to have a write function.
>> > As the ttynull console driver does not need any console output the write
>> > function was not implemented. This caused a "unable to handle kernel NULL
>> > pointer dereference" when the write function is called now.
>> > 
>> > To fix this issue, implement an empty write function.
>> > 
>> > Fixes: a699449bb13b ("printk: refactor and rework printing logic")
>> > Cc: stable@vger.kernel.org
>> > Signed-off-by: Michael Thalmeier <michael.thalmeier@hale.at>
>> 
>> Looking at the referenced commit, the only place I see it calling
>> con->write() is from call_console_driver(), which is in turn only called
>> from console_emit_next_record().  And console_flush_all(), the only
>> caller of console_emit_next_record(), checks that con->write is not NULL
>> using console_is_usable() before calling console_emit_next_record().
> 
> I see the same. The refactoring moved the check of con->write from
> call_console_driver() to console_is_usable(). It detects the NULL
> pointer earlier in console_flush_all()...
> 
>> What am I missing?  Which code path in the referenced commit calls
>> con->write without checking for NULL?
> 
> Vincent, could you please provide log with the backtrace?
> 
> I wonder if the problem is in the RT-patchset where
> console_emit_next_record() is called also from the printk kthread.

You are right. I have encountered this problem with the RT-patchset.
We currently are using the latest v5.15-rt kernel which had this problem.
 
> That said, the current code is error-prone. The check for non-NULL
> con->write is too far from the caller.
> 
> 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.
> 
> Anyway, I would like to understand if the "unable to handle kernel NULL
> pointer dereference" is a real problem in the current implementation.

Unfortunately I have not yet tested with the current RT or non-RT versions.

> 
> Best Regards,
> Petr

Regards, Michael

Re: [PATCH v2] tty: ttynull: implement console write

[John Ogness]

On 2023-02-15, Petr Mladek <pmladek@suse.com> wrote:
> 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()

Currently in linux-next it is:

   + console_flush_all()
     + console_emit_next_record()
       + 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.

It needs to call console_is_usable() for many reasons, not just the NULL
write() pointer check. call_console_driver() is already gone in
linux-next and calling console_emit_next_record() without first checking
if the console is allowed to print would be a clear bug. Perhaps we
should add kerneldoc to console_emit_next_record() mentioning that the
function assumes it is allowed to call con->write() (based on the many
checks in console_is_usable()).

The check for console usability was left outside of
console_emit_next_record() because that is important information needed
by console_flush_all(), not by console_emit_next_record(). In this case
console_is_usable() is being called not only to know if it can call
console_emit_next_record(), but also to know if any usable console
exists at all.

(Perhaps you want ttynull to be considered "usable"? But even if the
answer to that is "yes", then I would move the NULL check out of
console_is_usable() and into console_emit_next_record() so that it
reports success without performing any string-printing.)

> 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.

Checking a NULL pointer is a lot cheaper than string-printing a console
message to a buffer to send to a dummy function that does not even use
it.

> IMHO, normal console drivers would be useless without write()
> callback.

When atomic consoles are introduced there will be a second
write_atomic() function. Indeed, in the proposed code it is allowed for
either to be NULL because a console driver might be atomic-only or not
support atomic.

> 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.

It is common practice in most subsystems for callback functions that are
not implemented to be initialized to NULL. ttynull shows that there are
valid use cases. With atomic consoles there may be more. I would be more
inclined to NACK a driver with a dummy write()/write_atomic() callback
because it is misleading the subsystem and wasting real CPU cycles.

John Ogness

Re: [PATCH v2] tty: ttynull: implement console write

[John Ogness]

On 2023-02-15, Michael Thalmeier <michael.thalmeier@hale.at> wrote:
> You are right. I have encountered this problem with the RT-patchset.
> We currently are using the latest v5.15-rt kernel which had this
> problem.

The 5.15-rt kernel is based on an implementation of printk that has
since been abandoned. I will provide a patch for 5.15-rt to fix this
issue.

The new printk implementation is currently being finalized for inclusion
in the latest PREEMPT_RT-development version and for submission to
mainline.

John Ogness

Re: [PATCH v2] tty: ttynull: implement console write

[Greg Kroah-Hartman]

On Wed, Feb 15, 2023 at 11:17:58PM +0106, John Ogness wrote:
> On 2023-02-15, Michael Thalmeier <michael.thalmeier@hale.at> wrote:
> > You are right. I have encountered this problem with the RT-patchset.
> > We currently are using the latest v5.15-rt kernel which had this
> > problem.
> 
> The 5.15-rt kernel is based on an implementation of printk that has
> since been abandoned. I will provide a patch for 5.15-rt to fix this
> issue.

Ok, I'll drop this patch from my review queue as it's not needed in
mainline.

thanks,

greg k-h