linux-kernel - Re: [RFC V2] printk: add warning while drop partial text in msg

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: <CAOVJa8GND5AY=ruWQ8_H=utvwi5+-Xk0DgcU-MGeV0xagLKHgw@mail.gmail.com>
Date:   Fri, 11 Aug 2017 00:26:07 +0800
From:   pierre kuo <vichy.kuo@...il.com>
To:     Sergey Senozhatsky <sergey.senozhatsky.work@...il.com>
Cc:     Petr Mladek <pmladek@...e.com>,
        Sergey Senozhatsky <sergey.senozhatsky@...il.com>,
        rostedt@...dmis.org, linux-kernel@...r.kernel.org
Subject: Re: [RFC V2] printk: add warning while drop partial text in msg

hi Sergey
> this is not the only place that can truncate the message.
> vprintk_emit() can do so as well /* vscnprintf() */. but
> I think we don't care that much. a user likely will  notice
> truncated messages. we report lost messages, because this
> is a completely different sort of problem.
Usually people will more easily find message truncated from semantics
by vscnprintf,
since it brute force truncate input message by the upper limit of output buffer.

In msg_print_text, it use memchr to find "\n" as copying unit while
memcping msg->text to output buffer.
People will more difficultly to find out some part of message is left behind.
That is the reason I try to add such warning in msg_print_text.

>
>
> [..]
>> @@ -1264,8 +1270,23 @@ static size_t msg_print_text(const struct printk_log *msg, bool syslog, char *bu
>>
>>               if (buf) {
>>                       if (print_prefix(msg, syslog, NULL) +
>> -                         text_len + 1 >= size - len)
>> +                         text_len + 1 >= size - len) {
>> +                             /* below stores dropped characters
>> +                              * related information in next msg
>> +                              */
>> +                             size_t drop_len;
>> +
>> +                             drop_len = scnprintf(drop_msg,
>> +                                     MAX_DROP_MSG_LENGTH,
>> +                                     "<%u characters dropped>",
>> +                                     (next) ?
>> +                                     (unsigned int)(text_size + next - text) :
>> +                                     (unsigned int)text_size);
>> +                             drop_msg[drop_len] = 0;
>> +                             log_store(msg->facility, msg->level, msg->flags,
>> +                                     0, NULL, 0, drop_msg, strlen(drop_msg));
>>                               break;
>> +                     }
>
> this change, most likely, will confuse people. because msg_print_text() is
> called on a message that is being currently processed, which is not
> necessarily the last message in the logbuf. for example, see console_unlock().
> we do something like this:
>
>                 while (console_seq != log_next_seq) {
>                         msg = log_from_idx(console_idx);
>                         msg_print_text(msg);
>                         console_idx = log_next(console_idx);
>                         console_seq++;
>                 }
>
> your log_store(), invoked from msg_print_text(), will append the error
> message to the logbuf (tail), possibly far-far-far away from console_idx.
> so your "characters dropped" warning will appear much later.
>
>
>>                       len += print_prefix(msg, syslog, buf + len);
>>                       memcpy(buf + len, text, text_len);
>
>
> but more importantly, msg_print_text() is called from several places. and
> can even be called from a user space, potentially triggering the same
> "<characters dropped>" error log_store() over and over again, wiping out
> the actually important kernel messages. which is
>         a) not nice
> and
>         b) can be used to deliberately "hide" something really important.
>
>
> so, no. sorry, I don't like this change.
>
>         -ss