linux-kernel - Re: [PATCH] printk: Fix discarding of records

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]

Date:	Mon, 17 Feb 2014 00:05:15 +0100
From:	Kay Sievers <kay@...y.org>
To:	Greg Kroah-Hartman <gregkh@...uxfoundation.org>
Cc:	Linus Torvalds <torvalds@...ux-foundation.org>,
	Debabrata Banerjee <dbanerje@...mai.com>,
	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
	Jeff Mahoney <jeffm@...e.com>, dbavatar@...il.com,
	johunt@...mai.com, stable <stable@...r.kernel.org>
Subject: Re: [PATCH] printk: Fix discarding of records

On Sun, Feb 16, 2014 at 9:47 PM, Greg Kroah-Hartman
<gregkh@...uxfoundation.org> wrote:
> On Sun, Feb 16, 2014 at 11:28:36AM -0800, Linus Torvalds wrote:
>> Adding Kay and Greg, since the original code is from commit
>> 7ff9554bb578 ("printk: convert byte-buffer to variable-length record
>> buffer") and all the "prev" flag tweaks end up building on top of
>> that.
>>
>> The whole "prev flags" is messed up, and LOG_CONT is done very confusingly.
>>
>> Why are *those* particular two "prev = msg->flags" incorrect, when
>> every other case where we walk the messages they are required?
>>
>> The code/logic makes no sense. You remove the "prev = msg->flags" at
>> line 1070, when the *identical* loop just above it has it. So now the
>> two loops count the number of characters differently. That makes no
>> sense.
>>
>> So I don't think this fixes the fundamental problem. I'm more inclined
>> to believe that LOG_CONT is wrongly set somewhere, for example because
>> a continuation wasn't actually originally printed due to coming from
>> different users or something like that.
>>
>> Or at the very least I want a coherent explanation why one loop would
>> do this and the other would not, and why counting up *different*
>> numbers could possibly make sense.
>>
>> Because as it is, there clearly is some problem, but the patch does
>> not look sensible to me.
>
> Yeah, it doesn't make much sense to me either.
>
> Kay had a printk() test module that would stress these types of paths
> out a bunch, Kay, is that module around somewhere that we could maybe
> add it to the kernel tree so it could be used to test changes like this?

The module hack only tested the new stuff, not the legacy syslog() interface.

There is a bug in syslog_print_all(). We calculate the size of the
entire buffer for the plain text output including all the needed
headers, then we start to drop messages from the start, until the
result fits into the output buffer, and we start copying it.

Some of the lines get a header printed, cont lines don't. While
dropping messages from the start, if we end up at a line that did not
get a header added in the size calculation, we still add the header
now when copying it out as the first line.

We don't account for the size of this one additional header, and
therefore might end up writing up to 18 chars (syslog + time) to many.

I'll fix that tomorrow. Dropping the prev = in the loop avoids the
problem, but it will also result in fewer messages to be copied than
we could. We only have to account for the first header.

Thanks,
Kay
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/