From 36594b317c656bec8f968db93701d2cb9bc9155c Mon Sep 17 00:00:00 2001 From: Jia He Date: Fri, 9 Aug 2019 09:24:56 +0800 Subject: [PATCH 1/5] 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 2/5] 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 3/5] 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 4/5] 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 5/5] 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