Re: RFC: printk: kmsg_dump_get_line_nolock() buffer overflow

[Petr Mladek]

On Thu 2021-01-14 17:06:59, John Ogness wrote:
> On 2021-01-14, Petr Mladek <pmladek@xxxxxxxx> wrote:
> > It is pitty that I have missed this. I remember that I discussed
> > exactly this problem before, see
> > https://lore.kernel.org/lkml/20190710080402.ab3f4qfnvez6dhtc@xxxxxxxx/
> >
> > And I did exactly the same mistake. I have missed the two users in
> > "arch/powerpc" and "arch/um".
> >
> > It is clear that this problem happens repeatedly.
> 
> Yes, because the semantics are poor and undocumented.

I fully agree.


> > Now, the change in record_printk_text() behavior affects also other
> > callers. For example, syslog_print() fills the buffer completely
> > as well now. I could imagine a userspace code that does the same
> > mistake and it works just by chance.
> 
> No, syslog_print() works fine.

Not really. It fills the entire buffer provided by the user space now.
It never filled the last byte before. User space might do exactly
the same mistake:

	len = syslog(SYSLOG_ACTION_READ, buf, sizeof(*buf));
	buf[len] = '\0';

This worked before and it causes overflow now.

> There are only 2 users that think they
> can blindly add a byte at buffer[len]. Their code looks scary just
> seeing it.

Except that the functions are exported. I know that breaking external
modules that do ugly things might motivate them to upstream them but...


> > We should restore the original record_printk_text() behavior
> > and add the comment explaining why it is done this way.
> 
> OK.
> 
> > And I would even explicitly add the trailing '\0' as suggested at
> > https://lore.kernel.org/lkml/20190710121049.rwhk7fknfzn3cfkz@xxxxxxxxxxxxxxx/#t
> 
> OK. But then this becomes official semantics so powerpc/um no longer
> need to append a terminator.

We either risk the change of the semantic and break external code.

Or we should make it official. The '\0' will not be used by most of
the API users. But it will make it more safe. The free byte will be
there anyway.

Best Regards,
Petr