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]

Message-ID: <CA+55aFyiEcMBegoypWVTZXnb4aLj1PD4FRHWV83X2DY=8VHz7w@mail.gmail.com>
Date:	Sun, 16 Feb 2014 15:59:51 -0800
From:	Linus Torvalds <torvalds@...ux-foundation.org>
To:	"Banerjee, Debabrata" <dbanerje@...mai.com>
Cc:	Kay Sievers <kay.sievers@...y.org>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
	Jeff Mahoney <jeffm@...e.com>,
	"dbavatar@...il.com" <dbavatar@...il.com>,
	"Hunt, Joshua" <johunt@...mai.com>, stable <stable@...r.kernel.org>
Subject: Re: [PATCH] printk: Fix discarding of records

On Sun, Feb 16, 2014 at 3:23 PM, Banerjee, Debabrata
<dbanerje@...mai.com> wrote:
>
> The explanation is: the loops look identical but they are not. When a
> record is printed first, its size can expand due to adding the prefix and
> timestamp. The second loop is calculating len with the first line printed
> possibly changing every iteration.

That still makes zero sense.

The size should damn well not change, because we *should* be calling
it with the exact same flags. If the size changes, something is wrong.
The logic is:

 - first traverse all log indexes to find out the total length

 - then traverse *again* all the indexes, until you have traversed
enough that the remainder fits in the given buffer size.

For this to make sense, that second pass absolutely *has* to use the
exact same lengths as the first pass did. Otherwise the whole logic is
totally broken.

Now, *once* you have found the right record (so that the rest should
fit), the problem is that the *third* loop (that traverses that "rest"
part) is now done with a "prev" value that does not match the original
"let's figure out the size".

So my suspicion is that the *real* bug is that

    prev = 0;

before that *third* loop, because it means that the first time through
that loop (when we did *not* reset "seq/idx" to the beginning, we use
the wrong "prev" value.

So my (totally and utterly untested, and obviously whitespace-damaged)
suggested patch would be something along the lines of this:

    diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
    index b1d255f04135..053c3c1a4061 100644
    --- a/kernel/printk/printk.c
    +++ b/kernel/printk/printk.c
    @@ -1076,7 +1076,6 @@ static int syslog_print_all(char __user
*buf, int size, bool clear)
                    next_seq = log_next_seq;

                    len = 0;
    -               prev = 0;
                    while (len >= 0 && seq < next_seq) {
                            struct printk_log *msg = log_from_idx(idx);
                            int textlen;

because at least this makes sense. It means that "prev" is only zero
when we start from "clear_idx", at all other times it's actually the
msg->flags of the previous message.

But maybe I'm missing something. That code really is messy. But
clearing "prev" there in the middle really looks wrong to me.

               Linus
--
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/