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]

Date:   Tue, 1 Aug 2017 11:43:03 +0900
From:   Sergey Senozhatsky <sergey.senozhatsky.work@...il.com>
To:     pierre Kuo <vichy.kuo@...il.com>
Cc:     pmladek@...e.com, 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

On (07/30/17 21:37), pierre Kuo wrote:
> If the buffer pass to msg_print_text is not big enough to put both all
> prefixes and log_text(msg), kernel will quietly break.
> That means the user may not have the chance to know whether the
> log_text(msg) is fully printed into buffer or not.
> 
> In this patch, once above case happened, we try to calculate how many
> characters of log_text(msg) are dropped and add warning for debugging
> purpose.
[..]

Hello,

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.


[..]
> @@ -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