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
| ||
|
Message-ID: <53C7BF2E.4070705@linaro.org> Date: Thu, 17 Jul 2014 07:18:54 -0500 From: Alex Elder <elder@...aro.org> To: Petr Mládek <pmladek@...e.cz> CC: akpm@...ux-foundation.org, bp@...e.de, john.stultz@...aro.org, jack@...e.cz, linux-kernel@...r.kernel.org Subject: Re: [PATCH 3/4] printk: honor LOG_PREFIX in msg_print_text() On 07/17/2014 04:40 AM, Petr Mládek wrote: > On Wed 2014-07-16 12:26:59, Alex Elder wrote: >> This patch fixes a problem similar to what was addressed in the >> previous patch. >> >> All paths that read and format log records (for consoles, and for >> reading via syslog and /dev/kmsg) go through msg_print_text(). That >> function starts with some logic to determine whether the given log >> record when formatted should begin with a "prefix" string, and >> whether it should end with a newline. That logic has a bug. >> >> The LOG_PREFIX flag in a log record indicates that when it's >> formatted, a log record should include a prefix. This is used to >> force a record to begin a new line--even if its preceding log record >> contained LOG_CONT (indicating its content should be combined with >> the next record). >> >> Like the previous patch, the problem occurs when these flags are >> all set: >> prev & LOG_CONT >> msg->flags & LOG_PREFIX >> msg->flags & LOG_CONT >> In that case, "prefix" should be true but it is not. > > You are right. That's great news. >> The fix involves checking LOG_PREFIX when a message has its LOG_CONT >> flag set, and in that case allowing "prefix" to become false only >> if LOG_PREFIX is not set. I.e., the logic for "prefix" would become: >> >> if (prev & LOG_CONT && !(msg->flags & LOG_PREFIX)) >> prefix = false; >> if (msg->flags & LOG_CONT) >> if (prev & LOG_CONT && !(msg->flags & LOG_PREFIX)) >> prefix = false; >> >> However, note that this makes the (msg->flags & LOG_CONT) block >> redunant--it's handled by the test just above it. The result >> becomes quite a bit simpler than before. >> >> The following table concisely defines the problem: >> >> prev | msg | msg || >> CONT |PREFIX| CONT ||prefix >> ------+------+------++------ >> clear| clear| clear|| true >> clear| clear| set || true >> clear| set | clear|| true >> clear| set | set || true >> set | clear| clear||false >> set | set | set ||false clear (Same problem you pointed out in the next patch.) >> set | set | clear|| true >> set | set | set ||false <-- should be true >> >> Signed-off-by: Alex Elder <elder@...aro.org> >> --- >> kernel/printk/printk.c | 7 ++----- >> 1 file changed, 2 insertions(+), 5 deletions(-) >> >> diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c >> index 9e9cf93..3f15d95 100644 >> --- a/kernel/printk/printk.c >> +++ b/kernel/printk/printk.c >> @@ -1003,14 +1003,11 @@ static size_t msg_print_text(const struct printk_log *msg, enum log_flags prev, >> bool newline = true; >> size_t len = 0; >> >> - if ((prev & LOG_CONT) && !(msg->flags & LOG_PREFIX)) >> + if (prev & LOG_CONT && !(msg->flags & LOG_PREFIX)) >> prefix = false; > > I would personaly keep the brackets there. For me. > > ( & ) && !( & ) That's fine. I tend to be minimalist unless the compiler suggests otherwise, but I'll keep the parentheses. > is easier to parse than > > & && !( & ) > >> - if (msg->flags & LOG_CONT) { >> - if (prev & LOG_CONT) >> - prefix = false; >> + if (msg->flags & LOG_CONT) >> newline = false; >> - } > > You are right. The check before "prefix = false" did not make much > sense. We should not remove prefix just because the previous line was > continuous. Also it does not make sense to do this only when the new line > is continuous. I came at this trying to understand what was intended by reading the code. And it is not easy. These patches and the set that I'll post soon simplify things enormously. > But I think that the fix is not complete. IMHO, we should finish the > previous continuous line with '\n' before we print the prefix. I mean something > like: I will re-post a new version of this patch. When I do so I will look at this and--unless I find I disagree--will implement your suggestion. Thanks. -Alex > > if ((prev & LOG_CONT) && (msg->flags & LOG_PREFIX) && (len < size)) { > /* finish the incomplete continuous line */ > if (buf) { > buf[len++] = '\n'; > } else { > len++; > } > } > > Best Regards, > Petr > > >> >> do { >> const char *next = memchr(text, '\n', text_size); >> -- >> 1.9.1 >> -- 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/
Powered by blists - more mailing lists