linux-kernel - Re: [PATCH] printk: Do not lose last line in kmsg dump

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: <20190710084731.2e3caa29@gandalf.local.home>
Date:   Wed, 10 Jul 2019 08:47:31 -0400
From:   Steven Rostedt <rostedt@...dmis.org>
To:     Petr Mladek <pmladek@...e.com>
Cc:     Sergey Senozhatsky <sergey.senozhatsky.work@...il.com>,
        Vincent Whitchurch <vincent.whitchurch@...s.com>,
        "sergey.senozhatsky@...il.com" <sergey.senozhatsky@...il.com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] printk: Do not lose last line in kmsg dump

On Wed, 10 Jul 2019 14:10:49 +0200
Petr Mladek <pmladek@...e.com> wrote:

> On Wed 2019-07-10 17:19:22, Sergey Senozhatsky wrote:
 
> > > arch/powerpc/xmon/xmon.c
> > > 2836:	while (kmsg_dump_get_line_nolock(&dumper, false, buf, sizeof(buf), &len)) {
> > > 2837-		buf[len] = '\0';
> > > 
> > > arch/um/kernel/kmsg_dump.c
> > > 29:	while (kmsg_dump_get_line(dumper, true, line, sizeof(line), &len)) {
> > > 30-		line[len] = '\0';
> > > 
> > > I guess we should fix these first and leave this patch as is?  
> > 
> > We certainly need to fix something here, and I'd say that we
> > better handle it on the msg_print_text() side. There might be
> > more kmsg_dump_get_line() users doing `buf[len] = '\0'' in the
> > future.  

So basically the issues is that callers may expect that the return size
is still in the buffer and they append a '\0' to it. This is the same
issue with strncpy() and will cause the same kinds of bugs.

> 
> I though more about it and I agree with Sergey. One unused byte does
> not look worth the risk. Especially when we are talking about strings
> where many people expect the trailing '\0'.
> 
> I would even modify msg_print_text() to always add the trailing '\0'.
> All bytes will be used and it will be more error-proof API. I guess
> that this is what Sergey meant by better handling it on the
> msg_print_text() side.

I agree too. We either keep it as is and let the callers be able to add
the '\0', or we preferably add the '\0' ourselves and return the length
written (not counting the terminating '\0'), and we can then remove the
callers adding the '\0' later.

That's the safest approach.

-- Steve