From c9dccacfccc72c32692eedff4a27a4b0833a2afd Mon Sep 17 00:00:00 2001 From: Vincent Whitchurch Date: Thu, 11 Jul 2019 16:29:37 +0200 Subject: [PATCH 1/6] printk: Do not lose last line in kmsg buffer dump kmsg_dump_get_buffer() is supposed to select all the youngest log messages which fit into the provided buffer. It determines the correct start index by using msg_print_text() with a NULL buffer to calculate the size of each entry. However, when performing the actual writes, msg_print_text() only writes the entry to the buffer if the written len is lesser than the size of the buffer. So if the lengths of the selected youngest log messages happen to precisely fill up the provided buffer, the last log message is not included. We don't want to modify msg_print_text() to fill up the buffer and start returning a length which is equal to the size of the buffer, since callers of its other users, such as kmsg_dump_get_line(), depend upon the current behaviour. Instead, fix kmsg_dump_get_buffer() to compensate for this. For example, with the following two final prints: [ 6.427502] AAAAAAAAAAAAA [ 6.427769] BBBBBBBB12345 A dump of a 64-byte buffer filled by kmsg_dump_get_buffer(), before this patch: 00000000: 3c 30 3e 5b 20 20 20 20 36 2e 35 32 32 31 39 37 <0>[ 6.522197 00000010: 5d 20 41 41 41 41 41 41 41 41 41 41 41 41 41 0a ] AAAAAAAAAAAAA. 00000020: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................ 00000030: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................ After this patch: 00000000: 3c 30 3e 5b 20 20 20 20 36 2e 34 35 36 36 37 38 <0>[ 6.456678 00000010: 5d 20 42 42 42 42 42 42 42 42 31 32 33 34 35 0a ] BBBBBBBB12345. 00000020: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................ 00000030: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................ Link: http://lkml.kernel.org/r/20190711142937.4083-1-vincent.whitchurch@axis.com Fixes: e2ae715d66bf4bec ("kmsg - kmsg_dump() use iterator to receive log buffer content") To: rostedt@goodmis.org Cc: linux-kernel@vger.kernel.org Cc: # v3.5+ Signed-off-by: Vincent Whitchurch Reviewed-by: Sergey Senozhatsky Signed-off-by: Petr Mladek --- kernel/printk/printk.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c index 1888f6a3b694..424abf802f02 100644 --- a/kernel/printk/printk.c +++ b/kernel/printk/printk.c @@ -3274,7 +3274,7 @@ bool kmsg_dump_get_buffer(struct kmsg_dumper *dumper, bool syslog, /* move first record forward until length fits into the buffer */ seq = dumper->cur_seq; idx = dumper->cur_idx; - while (l > size && seq < dumper->next_seq) { + while (l >= size && seq < dumper->next_seq) { struct printk_log *msg = log_from_idx(idx); l -= msg_print_text(msg, true, time, NULL, 0); From 36594b317c656bec8f968db93701d2cb9bc9155c Mon Sep 17 00:00:00 2001 From: Jia He Date: Fri, 9 Aug 2019 09:24:56 +0800 Subject: [PATCH 2/6] vsprintf: Prevent crash when dereferencing invalid pointers for %pD Commit 3e5903eb9cff ("vsprintf: Prevent crash when dereferencing invalid pointers") prevents most crash except for %pD. There is an additional pointer dereferencing before dentry_name. At least, vma->file can be NULL and be passed to printk %pD in print_bad_pte, which can cause crash. This patch fixes it with introducing a new file_dentry_name. Link: http://lkml.kernel.org/r/20190809012457.56685-1-justin.he@arm.com Fixes: 3e5903eb9cff ("vsprintf: Prevent crash when dereferencing invalid pointers") To: Geert Uytterhoeven To: Thomas Gleixner To: Andy Shevchenko To: linux-kernel@vger.kernel.org Cc: Kees Cook Cc: "Steven Rostedt (VMware)" Cc: Shuah Khan Cc: "Tobin C. Harding" Signed-off-by: Jia He Reviewed-by: Andy Shevchenko Reviewed-by: Sergey Senozhatsky Signed-off-by: Petr Mladek --- lib/vsprintf.c | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/lib/vsprintf.c b/lib/vsprintf.c index b0967cf17137..e78017a3e1bd 100644 --- a/lib/vsprintf.c +++ b/lib/vsprintf.c @@ -869,6 +869,15 @@ char *dentry_name(char *buf, char *end, const struct dentry *d, struct printf_sp return widen_string(buf, n, end, spec); } +static noinline_for_stack +char *file_dentry_name(char *buf, char *end, const struct file *f, + struct printf_spec spec, const char *fmt) +{ + if (check_pointer(&buf, end, f, spec)) + return buf; + + return dentry_name(buf, end, f->f_path.dentry, spec, fmt); +} #ifdef CONFIG_BLOCK static noinline_for_stack char *bdev_name(char *buf, char *end, struct block_device *bdev, @@ -2166,9 +2175,7 @@ char *pointer(const char *fmt, char *buf, char *end, void *ptr, case 'C': return clock(buf, end, ptr, spec, fmt); case 'D': - return dentry_name(buf, end, - ((const struct file *)ptr)->f_path.dentry, - spec, fmt); + return file_dentry_name(buf, end, ptr, spec, fmt); #ifdef CONFIG_BLOCK case 'g': return bdev_name(buf, end, ptr, spec, fmt); From cf6b7921fc19e537cd5ae88460195c8599eb5d9d Mon Sep 17 00:00:00 2001 From: Jia He Date: Fri, 9 Aug 2019 09:24:57 +0800 Subject: [PATCH 3/6] lib/test_printf: Add test of null/invalid pointer dereference for dentry This add some additional test cases of null/invalid pointer dereference for dentry and file (%pd and %pD) Link: http://lkml.kernel.org/r/20190809012457.56685-2-justin.he@arm.com To: Geert Uytterhoeven To: Sergey Senozhatsky To: Thomas Gleixner To: Andy Shevchenko To: Petr Mladek To: linux-kernel@vger.kernel.org Cc: Kees Cook Cc: "Steven Rostedt (VMware)" Cc: Shuah Khan Cc: "Tobin C. Harding" Signed-off-by: Jia He Reviewed-by: Andy Shevchenko Reviewed-by: Sergey Senozhatsky Signed-off-by: Petr Mladek --- lib/test_printf.c | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/lib/test_printf.c b/lib/test_printf.c index 944eb50f3862..befedffeb476 100644 --- a/lib/test_printf.c +++ b/lib/test_printf.c @@ -455,6 +455,13 @@ dentry(void) test("foo", "%pd", &test_dentry[0]); test("foo", "%pd2", &test_dentry[0]); + /* test the null/invalid pointer case for dentry */ + test("(null)", "%pd", NULL); + test("(efault)", "%pd", PTR_INVALID); + /* test the null/invalid pointer case for file */ + test("(null)", "%pD", NULL); + test("(efault)", "%pD", PTR_INVALID); + test("romeo", "%pd", &test_dentry[3]); test("alfa/romeo", "%pd2", &test_dentry[3]); test("bravo/alfa/romeo", "%pd3", &test_dentry[3]); From 8ebea6ea1a7ed5d67ecbb2a493c716a2a89c0be2 Mon Sep 17 00:00:00 2001 From: Petr Mladek Date: Thu, 15 Aug 2019 17:01:33 +0200 Subject: [PATCH 4/6] lib/test_printf: Remove obvious comments from %pd and %pD tests Signed-off-by: Petr Mladek --- lib/test_printf.c | 2 -- 1 file changed, 2 deletions(-) diff --git a/lib/test_printf.c b/lib/test_printf.c index befedffeb476..5d94cbff2120 100644 --- a/lib/test_printf.c +++ b/lib/test_printf.c @@ -455,10 +455,8 @@ dentry(void) test("foo", "%pd", &test_dentry[0]); test("foo", "%pd2", &test_dentry[0]); - /* test the null/invalid pointer case for dentry */ test("(null)", "%pd", NULL); test("(efault)", "%pd", PTR_INVALID); - /* test the null/invalid pointer case for file */ test("(null)", "%pD", NULL); test("(efault)", "%pD", PTR_INVALID); From 35c35493b0e3343522256f6054516f4e2161a6d4 Mon Sep 17 00:00:00 2001 From: Chuhong Yuan Date: Fri, 9 Aug 2019 15:10:34 +0800 Subject: [PATCH 5/6] printk: Replace strncmp() with str_has_prefix() strncmp(str, const, len) is error-prone because len is easy to have typo. An example is the hard-coded len has counting error or sizeof(const) forgets - 1. So we prefer using newly introduced str_has_prefix() to substitute such strncmp() to make code better. Link: http://lkml.kernel.org/r/20190809071034.17279-1-hslester96@gmail.com Cc: "Steven Rostedt" Cc: linux-kernel@vger.kernel.org Signed-off-by: Chuhong Yuan Reviewed-by: Sergey Senozhatsky [pmladek@suse.com: Slightly updated and reformatted the commit message.] Signed-off-by: Petr Mladek --- kernel/printk/braille.c | 15 +++++++++++---- kernel/printk/printk.c | 26 ++++++++++++++++++-------- 2 files changed, 29 insertions(+), 12 deletions(-) diff --git a/kernel/printk/braille.c b/kernel/printk/braille.c index 1d21ebacfdb8..17a9591e54ff 100644 --- a/kernel/printk/braille.c +++ b/kernel/printk/braille.c @@ -11,11 +11,18 @@ int _braille_console_setup(char **str, char **brl_options) { - if (!strncmp(*str, "brl,", 4)) { + size_t len; + + len = str_has_prefix(*str, "brl,"); + if (len) { *brl_options = ""; - *str += 4; - } else if (!strncmp(*str, "brl=", 4)) { - *brl_options = *str + 4; + *str += len; + return 0; + } + + len = str_has_prefix(*str, "brl="); + if (len) { + *brl_options = *str + len; *str = strchr(*brl_options, ','); if (!*str) { pr_err("need port name after brl=\n"); diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c index 1888f6a3b694..43a31015ec93 100644 --- a/kernel/printk/printk.c +++ b/kernel/printk/printk.c @@ -118,19 +118,29 @@ static unsigned int __read_mostly devkmsg_log = DEVKMSG_LOG_MASK_DEFAULT; static int __control_devkmsg(char *str) { + size_t len; + if (!str) return -EINVAL; - if (!strncmp(str, "on", 2)) { + len = str_has_prefix(str, "on"); + if (len) { devkmsg_log = DEVKMSG_LOG_MASK_ON; - return 2; - } else if (!strncmp(str, "off", 3)) { - devkmsg_log = DEVKMSG_LOG_MASK_OFF; - return 3; - } else if (!strncmp(str, "ratelimit", 9)) { - devkmsg_log = DEVKMSG_LOG_MASK_DEFAULT; - return 9; + return len; } + + len = str_has_prefix(str, "off"); + if (len) { + devkmsg_log = DEVKMSG_LOG_MASK_OFF; + return len; + } + + len = str_has_prefix(str, "ratelimit"); + if (len) { + devkmsg_log = DEVKMSG_LOG_MASK_DEFAULT; + return len; + } + return -EINVAL; } From 085a3a8fdf3e2fbd4678dbeccbb656bd328b3715 Mon Sep 17 00:00:00 2001 From: James Byrne Date: Mon, 2 Sep 2019 11:18:16 +0000 Subject: [PATCH 6/6] ABI: Update dev-kmsg documentation to match current kernel behaviour Commit 5aa068ea4082 ("printk: remove games with previous record flags") abolished the practice of setting the log flag to 'c' for the first continuation line and '+' for subsequent lines. Now all continuation lines are flagged with 'c' and '+' is never used. Update the 'dev-kmsg' documentation to remove the reference to the obsolete '+' flag. In addition, state explicitly that only 8 bits of the syslog prefix are used for the facility number when writing to /dev/kmsg. Link: http://lkml.kernel.org/r/0102016cf1b26630-8e9b337b-da49-43c6-b028-4250c2fac3ef-000000@eu-west-1.amazonses.com Cc: Steven Rostedt Cc: linux-kernel@vger.kernel.org Signed-off-by: James Byrne Reviewed-by: Sergey Senozhatsky Signed-off-by: Petr Mladek --- Documentation/ABI/testing/dev-kmsg | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-) diff --git a/Documentation/ABI/testing/dev-kmsg b/Documentation/ABI/testing/dev-kmsg index fff817efa508..f307506eb54c 100644 --- a/Documentation/ABI/testing/dev-kmsg +++ b/Documentation/ABI/testing/dev-kmsg @@ -12,7 +12,7 @@ Description: The /dev/kmsg character device node provides userspace access The logged line can be prefixed with a syslog prefix, which carries the syslog priority and facility. The single decimal prefix number is composed of the 3 lowest bits being the syslog - priority and the higher bits the syslog facility number. + priority and the next 8 bits the syslog facility number. If no prefix is given, the priority number is the default kernel log priority and the facility number is set to LOG_USER (1). It @@ -90,13 +90,12 @@ Description: The /dev/kmsg character device node provides userspace access +sound:card0 - subsystem:devname The flags field carries '-' by default. A 'c' indicates a - fragment of a line. All following fragments are flagged with - '+'. Note, that these hints about continuation lines are not - necessarily correct, and the stream could be interleaved with - unrelated messages, but merging the lines in the output - usually produces better human readable results. A similar - logic is used internally when messages are printed to the - console, /proc/kmsg or the syslog() syscall. + fragment of a line. Note, that these hints about continuation + lines are not necessarily correct, and the stream could be + interleaved with unrelated messages, but merging the lines in + the output usually produces better human readable results. A + similar logic is used internally when messages are printed to + the console, /proc/kmsg or the syslog() syscall. By default, kernel tries to avoid fragments by concatenating when it can and fragments are rare; however, when extended