linux-kernel - Re: RFC: printk: kmsg_dump_get_line

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] [day] [month] [year] [list]

Message-ID: <YAB0FfMLfkrRd4uT@alley>
Date:   Thu, 14 Jan 2021 17:40:53 +0100
From:   Petr Mladek <pmladek@...e.com>
To:     John Ogness <john.ogness@...utronix.de>
Cc:     Sergey Senozhatsky <sergey.senozhatsky.work@...il.com>,
        Sergey Senozhatsky <sergey.senozhatsky@...il.com>,
        Steven Rostedt <rostedt@...dmis.org>,
        Linus Torvalds <torvalds@...ux-foundation.org>,
        linux-kernel@...r.kernel.org
Subject: Re: RFC: printk: kmsg_dump_get_line_nolock() buffer overflow

On Thu 2021-01-14 17:06:59, John Ogness wrote:
> On 2021-01-14, Petr Mladek <pmladek@...e.com> 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@axis.com/
> >
> > 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@pathway.suse.cz/#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