From: Steven Rostedt <rostedt@goodmis.org> To: Petr Mladek <pmladek@suse.com> Cc: Linus Torvalds <torvalds@linux-foundation.org>, Andy Shevchenko <andriy.shevchenko@linux.intel.com>, Rasmus Villemoes <linux@rasmusvillemoes.dk>, "Tobin C . Harding" <me@tobin.cc>, Michal Hocko <mhocko@suse.cz>, Sergey Senozhatsky <sergey.senozhatsky@gmail.com>, Sergey Senozhatsky <sergey.senozhatsky.work@gmail.com>, linux-kernel@vger.kernel.org, Michael Ellerman <mpe@ellerman.id.au>, linuxppc-dev@lists.ozlabs.org, Russell Currey <ruscur@russell.cc>, Christophe Leroy <christophe.leroy@c-s.fr>, Stephen Rothwell <sfr@ozlabs.org>, Heiko Carstens <heiko.carstens@de.ibm.com>, linux-arch@vger.kernel.org, linux-s390@vger.kernel.org, Martin Schwidefsky <schwidefsky@de.ibm.com> Subject: Re: [PATCH] vsprintf: Do not break early boot with probing addresses Date: Thu, 9 May 2019 09:13:57 -0400 [thread overview] Message-ID: <20190509091357.0af3fcd7@gandalf.local.home> (raw) In-Reply-To: <20190509121923.8339-1-pmladek@suse.com> On Thu, 9 May 2019 14:19:23 +0200 Petr Mladek <pmladek@suse.com> wrote: > The commit 3e5903eb9cff70730 ("vsprintf: Prevent crash when dereferencing > invalid pointers") broke boot on several architectures. The common > pattern is that probe_kernel_read() is not working during early > boot because userspace access framework is not ready. > > The check is only the best effort. Let's not rush with it during > the early boot. > > Details: > > 1. Report on Power: > > Kernel crashes very early during boot with with CONFIG_PPC_KUAP and > CONFIG_JUMP_LABEL_FEATURE_CHECK_DEBUG > > The problem is the combination of some new code called via printk(), > check_pointer() which calls probe_kernel_read(). That then calls > allow_user_access() (PPC_KUAP) and that uses mmu_has_feature() too early > (before we've patched features). With the JUMP_LABEL debug enabled that > causes us to call printk() & dump_stack() and we end up recursing and > overflowing the stack. > > Because it happens so early you don't get any output, just an apparently > dead system. > > The stack trace (which you don't see) is something like: > > ... > dump_stack+0xdc > probe_kernel_read+0x1a4 > check_pointer+0x58 > string+0x3c > vsnprintf+0x1bc > vscnprintf+0x20 > printk_safe_log_store+0x7c > printk+0x40 > dump_stack_print_info+0xbc > dump_stack+0x8 > probe_kernel_read+0x1a4 > probe_kernel_read+0x19c > check_pointer+0x58 > string+0x3c > vsnprintf+0x1bc > vscnprintf+0x20 > vprintk_store+0x6c > vprintk_emit+0xec > vprintk_func+0xd4 > printk+0x40 > cpufeatures_process_feature+0xc8 > scan_cpufeatures_subnodes+0x380 > of_scan_flat_dt_subnodes+0xb4 > dt_cpu_ftrs_scan_callback+0x158 > of_scan_flat_dt+0xf0 > dt_cpu_ftrs_scan+0x3c > early_init_devtree+0x360 > early_setup+0x9c > > 2. Report on s390: > > vsnprintf invocations, are broken on s390. For example, the early boot > output now looks like this where the first (efault) should be > the linux_banner: > > [ 0.099985] (efault) > [ 0.099985] setup: Linux is running as a z/VM guest operating system in 64-bit mode > [ 0.100066] setup: The maximum memory size is 8192MB > [ 0.100070] cma: Reserved 4 MiB at (efault) > [ 0.100100] numa: NUMA mode: (efault) > > The reason for this, is that the code assumes that > probe_kernel_address() works very early. This however is not true on > at least s390. Uaccess on KERNEL_DS works only after page tables have > been setup on s390, which happens with setup_arch()->paging_init(). > > Any probe_kernel_address() invocation before that will return -EFAULT. Hmm, this sounds to me that probe_kernel_address() is broken for these architectures. Perhaps the system_state check should be in probe_kernel_address() for those architectures? > > Fixes: 3e5903eb9cff70730 ("vsprintf: Prevent crash when dereferencing invalid pointers") > Signed-off-by: Petr Mladek <pmladek@suse.com> > --- > lib/vsprintf.c | 9 +++++++-- > 1 file changed, 7 insertions(+), 2 deletions(-) > > diff --git a/lib/vsprintf.c b/lib/vsprintf.c > index 7b0a6140bfad..8b43a883be6b 100644 > --- a/lib/vsprintf.c > +++ b/lib/vsprintf.c > @@ -640,8 +640,13 @@ static const char *check_pointer_msg(const void *ptr) > if (!ptr) > return "(null)"; > > - if (probe_kernel_address(ptr, byte)) > - return "(efault)"; Either that, or we add a macro to those architectures and add: #ifdef ARCH_NO_EARLY_PROBE_KERNEL_ADDRESS > + /* User space address handling is not ready during early boot. */ > + if (system_state <= SYSTEM_BOOTING) { > + if ((unsigned long)ptr < PAGE_SIZE) > + return "(efault)"; > + } else { #endif Why add an unnecessary branch for archs this is not a problem for? -- Steve > + if (probe_kernel_address(ptr, byte)) > + return "(efault)"; > > return NULL; > }
WARNING: multiple messages have this Message-ID (diff)
From: Steven Rostedt <rostedt@goodmis.org> To: Petr Mladek <pmladek@suse.com> Cc: linux-arch@vger.kernel.org, Sergey Senozhatsky <sergey.senozhatsky.work@gmail.com>, Heiko Carstens <heiko.carstens@de.ibm.com>, linux-s390@vger.kernel.org, linuxppc-dev@lists.ozlabs.org, Rasmus Villemoes <linux@rasmusvillemoes.dk>, linux-kernel@vger.kernel.org, Michal Hocko <mhocko@suse.cz>, Sergey Senozhatsky <sergey.senozhatsky@gmail.com>, Stephen Rothwell <sfr@ozlabs.org>, Andy Shevchenko <andriy.shevchenko@linux.intel.com>, Linus Torvalds <torvalds@linux-foundation.org>, Martin Schwidefsky <schwidefsky@de.ibm.com>, "Tobin C . Harding" <me@tobin.cc> Subject: Re: [PATCH] vsprintf: Do not break early boot with probing addresses Date: Thu, 9 May 2019 09:13:57 -0400 [thread overview] Message-ID: <20190509091357.0af3fcd7@gandalf.local.home> (raw) In-Reply-To: <20190509121923.8339-1-pmladek@suse.com> On Thu, 9 May 2019 14:19:23 +0200 Petr Mladek <pmladek@suse.com> wrote: > The commit 3e5903eb9cff70730 ("vsprintf: Prevent crash when dereferencing > invalid pointers") broke boot on several architectures. The common > pattern is that probe_kernel_read() is not working during early > boot because userspace access framework is not ready. > > The check is only the best effort. Let's not rush with it during > the early boot. > > Details: > > 1. Report on Power: > > Kernel crashes very early during boot with with CONFIG_PPC_KUAP and > CONFIG_JUMP_LABEL_FEATURE_CHECK_DEBUG > > The problem is the combination of some new code called via printk(), > check_pointer() which calls probe_kernel_read(). That then calls > allow_user_access() (PPC_KUAP) and that uses mmu_has_feature() too early > (before we've patched features). With the JUMP_LABEL debug enabled that > causes us to call printk() & dump_stack() and we end up recursing and > overflowing the stack. > > Because it happens so early you don't get any output, just an apparently > dead system. > > The stack trace (which you don't see) is something like: > > ... > dump_stack+0xdc > probe_kernel_read+0x1a4 > check_pointer+0x58 > string+0x3c > vsnprintf+0x1bc > vscnprintf+0x20 > printk_safe_log_store+0x7c > printk+0x40 > dump_stack_print_info+0xbc > dump_stack+0x8 > probe_kernel_read+0x1a4 > probe_kernel_read+0x19c > check_pointer+0x58 > string+0x3c > vsnprintf+0x1bc > vscnprintf+0x20 > vprintk_store+0x6c > vprintk_emit+0xec > vprintk_func+0xd4 > printk+0x40 > cpufeatures_process_feature+0xc8 > scan_cpufeatures_subnodes+0x380 > of_scan_flat_dt_subnodes+0xb4 > dt_cpu_ftrs_scan_callback+0x158 > of_scan_flat_dt+0xf0 > dt_cpu_ftrs_scan+0x3c > early_init_devtree+0x360 > early_setup+0x9c > > 2. Report on s390: > > vsnprintf invocations, are broken on s390. For example, the early boot > output now looks like this where the first (efault) should be > the linux_banner: > > [ 0.099985] (efault) > [ 0.099985] setup: Linux is running as a z/VM guest operating system in 64-bit mode > [ 0.100066] setup: The maximum memory size is 8192MB > [ 0.100070] cma: Reserved 4 MiB at (efault) > [ 0.100100] numa: NUMA mode: (efault) > > The reason for this, is that the code assumes that > probe_kernel_address() works very early. This however is not true on > at least s390. Uaccess on KERNEL_DS works only after page tables have > been setup on s390, which happens with setup_arch()->paging_init(). > > Any probe_kernel_address() invocation before that will return -EFAULT. Hmm, this sounds to me that probe_kernel_address() is broken for these architectures. Perhaps the system_state check should be in probe_kernel_address() for those architectures? > > Fixes: 3e5903eb9cff70730 ("vsprintf: Prevent crash when dereferencing invalid pointers") > Signed-off-by: Petr Mladek <pmladek@suse.com> > --- > lib/vsprintf.c | 9 +++++++-- > 1 file changed, 7 insertions(+), 2 deletions(-) > > diff --git a/lib/vsprintf.c b/lib/vsprintf.c > index 7b0a6140bfad..8b43a883be6b 100644 > --- a/lib/vsprintf.c > +++ b/lib/vsprintf.c > @@ -640,8 +640,13 @@ static const char *check_pointer_msg(const void *ptr) > if (!ptr) > return "(null)"; > > - if (probe_kernel_address(ptr, byte)) > - return "(efault)"; Either that, or we add a macro to those architectures and add: #ifdef ARCH_NO_EARLY_PROBE_KERNEL_ADDRESS > + /* User space address handling is not ready during early boot. */ > + if (system_state <= SYSTEM_BOOTING) { > + if ((unsigned long)ptr < PAGE_SIZE) > + return "(efault)"; > + } else { #endif Why add an unnecessary branch for archs this is not a problem for? -- Steve > + if (probe_kernel_address(ptr, byte)) > + return "(efault)"; > > return NULL; > }
next prev parent reply other threads:[~2019-05-09 13:14 UTC|newest] Thread overview: 95+ messages / expand[flat|nested] mbox.gz Atom feed top 2019-05-09 12:19 [PATCH] vsprintf: Do not break early boot with probing addresses Petr Mladek 2019-05-09 12:19 ` Petr Mladek 2019-05-09 13:05 ` Andy Shevchenko 2019-05-09 13:05 ` Andy Shevchenko 2019-05-09 13:13 ` Steven Rostedt [this message] 2019-05-09 13:13 ` Steven Rostedt 2019-05-09 14:06 ` Petr Mladek 2019-05-09 14:06 ` Petr Mladek 2019-05-09 13:38 ` Michal Suchánek 2019-05-09 13:38 ` Michal Suchánek 2019-05-09 13:46 ` David Laight 2019-05-09 13:46 ` David Laight 2019-05-10 10:21 ` Michael Ellerman 2019-05-10 10:21 ` Michael Ellerman 2019-05-10 10:21 ` Michael Ellerman 2019-05-10 4:32 ` Sergey Senozhatsky 2019-05-10 4:32 ` Sergey Senozhatsky 2019-05-10 4:47 ` Linus Torvalds 2019-05-10 5:07 ` Sergey Senozhatsky 2019-05-10 5:07 ` Sergey Senozhatsky 2019-05-10 6:41 ` Michael Ellerman 2019-05-10 6:41 ` Michael Ellerman 2019-05-10 6:41 ` Michael Ellerman 2019-05-10 6:41 ` Michael Ellerman 2019-05-10 8:06 ` Petr Mladek 2019-05-10 8:06 ` Petr Mladek 2019-05-10 8:16 ` Sergey Senozhatsky 2019-05-10 8:16 ` Sergey Senozhatsky 2019-05-10 8:42 ` Petr Mladek 2019-05-10 8:42 ` Petr Mladek 2019-05-10 8:51 ` Sergey Senozhatsky 2019-05-10 8:51 ` Sergey Senozhatsky 2019-05-10 14:49 ` Petr Mladek 2019-05-10 14:49 ` Petr Mladek 2019-05-10 16:24 ` Steven Rostedt 2019-05-10 16:24 ` Steven Rostedt 2019-05-10 16:32 ` Martin Schwidefsky 2019-05-10 16:32 ` Martin Schwidefsky 2019-05-10 16:40 ` Steven Rostedt 2019-05-10 16:40 ` Steven Rostedt 2019-05-10 16:45 ` Martin Schwidefsky 2019-05-10 16:45 ` Martin Schwidefsky 2019-05-13 12:24 ` Petr Mladek 2019-05-13 12:24 ` Petr Mladek 2019-05-10 16:41 ` Andy Shevchenko 2019-05-10 16:41 ` Andy Shevchenko 2019-05-10 17:35 ` christophe leroy 2019-05-10 17:35 ` christophe leroy 2019-05-13 8:52 ` David Laight 2019-05-13 8:52 ` David Laight 2019-05-13 9:13 ` Andy Shevchenko 2019-05-13 9:13 ` Andy Shevchenko 2019-05-13 9:13 ` Andy Shevchenko 2019-05-13 12:42 ` Petr Mladek 2019-05-13 12:42 ` Petr Mladek 2019-05-13 12:42 ` Petr Mladek 2019-05-13 14:15 ` Steven Rostedt 2019-05-13 14:15 ` Steven Rostedt 2019-05-13 14:15 ` Steven Rostedt 2019-05-14 2:07 ` Sergey Senozhatsky 2019-05-14 2:07 ` Sergey Senozhatsky 2019-05-14 2:07 ` Sergey Senozhatsky 2019-05-14 2:25 ` Sergey Senozhatsky 2019-05-14 2:25 ` Sergey Senozhatsky 2019-05-14 2:25 ` Sergey Senozhatsky 2019-05-14 8:28 ` David Laight 2019-05-14 8:28 ` David Laight 2019-05-14 8:28 ` David Laight 2019-05-14 9:02 ` Geert Uytterhoeven 2019-05-14 9:02 ` Geert Uytterhoeven 2019-05-14 9:02 ` Geert Uytterhoeven 2019-05-14 18:37 ` Steven Rostedt 2019-05-14 18:37 ` Steven Rostedt 2019-05-14 18:37 ` Steven Rostedt 2019-05-14 19:13 ` Geert Uytterhoeven 2019-05-14 19:13 ` Geert Uytterhoeven 2019-05-14 19:13 ` Geert Uytterhoeven 2019-05-14 19:35 ` Steven Rostedt 2019-05-14 19:35 ` Steven Rostedt 2019-05-14 19:35 ` Steven Rostedt 2019-05-15 7:23 ` Geert Uytterhoeven 2019-05-15 7:23 ` Geert Uytterhoeven 2019-05-15 7:23 ` Geert Uytterhoeven 2019-05-15 7:53 ` Petr Mladek 2019-05-15 7:53 ` Petr Mladek 2019-05-15 7:53 ` Petr Mladek 2019-05-15 6:21 ` Sergey Senozhatsky 2019-05-15 6:21 ` Sergey Senozhatsky 2019-05-15 6:21 ` Sergey Senozhatsky 2019-05-15 7:35 ` Petr Mladek 2019-05-15 7:35 ` Petr Mladek 2019-05-15 7:35 ` Petr Mladek 2019-05-15 9:00 ` David Laight 2019-05-15 9:00 ` David Laight 2019-05-15 9:00 ` David Laight
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20190509091357.0af3fcd7@gandalf.local.home \ --to=rostedt@goodmis.org \ --cc=andriy.shevchenko@linux.intel.com \ --cc=christophe.leroy@c-s.fr \ --cc=heiko.carstens@de.ibm.com \ --cc=linux-arch@vger.kernel.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-s390@vger.kernel.org \ --cc=linux@rasmusvillemoes.dk \ --cc=linuxppc-dev@lists.ozlabs.org \ --cc=me@tobin.cc \ --cc=mhocko@suse.cz \ --cc=mpe@ellerman.id.au \ --cc=pmladek@suse.com \ --cc=ruscur@russell.cc \ --cc=schwidefsky@de.ibm.com \ --cc=sergey.senozhatsky.work@gmail.com \ --cc=sergey.senozhatsky@gmail.com \ --cc=sfr@ozlabs.org \ --cc=torvalds@linux-foundation.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.