All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] x86/process: don't mix user/kernel regs in 64bit __show_regs
@ 2018-08-31 19:41 Jann Horn
  2018-08-31 20:11 ` Andy Lutomirski
  2018-09-06 12:36 ` [tip:x86/urgent] x86/process: Don't mix user/kernel regs in 64bit __show_regs() tip-bot for Jann Horn
  0 siblings, 2 replies; 5+ messages in thread
From: Jann Horn @ 2018-08-31 19:41 UTC (permalink / raw)
  To: Thomas Gleixner, Ingo Molnar, H. Peter Anvin, x86,
	Andy Lutomirski, jannh
  Cc: Borislav Petkov, Greg Kroah-Hartman, linux-kernel

When the kernel.print-fatal-signals sysctl has been enabled (I don't know
whether anyone actually enables it), a simple userspace crash will cause
the kernel to write a crash dump that contains, among other things, the
kernel gsbase into dmesg.

As suggested by Andy, limit output to pt_regs, FS_BASE and KERNEL_GS_BASE
in this case.

This also moves the bitness-specific logic from show_regs() into
process_{32,64}.c.

Signed-off-by: Jann Horn <jannh@google.com>
Fixes: 45807a1df9f5 ("vdso: print fatal signals")
---
@Andy: Does this look like what you had in mind?

Does this need a CC stable tag? I haven't put one on it for now.

 arch/x86/include/asm/kdebug.h | 12 +++++++++++-
 arch/x86/kernel/dumpstack.c   | 11 +++--------
 arch/x86/kernel/process_32.c  |  4 ++--
 arch/x86/kernel/process_64.c  | 12 ++++++++++--
 4 files changed, 26 insertions(+), 13 deletions(-)

diff --git a/arch/x86/include/asm/kdebug.h b/arch/x86/include/asm/kdebug.h
index 395c9631e000..75f1e35e7c15 100644
--- a/arch/x86/include/asm/kdebug.h
+++ b/arch/x86/include/asm/kdebug.h
@@ -22,10 +22,20 @@ enum die_val {
 	DIE_NMIUNKNOWN,
 };
 
+enum show_regs_mode {
+	SHOW_REGS_SHORT,
+	/*
+	 * For when userspace crashed, but we don't think it's our fault, and
+	 * therefore don't print kernel registers.
+	 */
+	SHOW_REGS_USER,
+	SHOW_REGS_ALL
+};
+
 extern void die(const char *, struct pt_regs *,long);
 extern int __must_check __die(const char *, struct pt_regs *, long);
 extern void show_stack_regs(struct pt_regs *regs);
-extern void __show_regs(struct pt_regs *regs, int all);
+extern void __show_regs(struct pt_regs *regs, enum show_regs_mode);
 extern void show_iret_regs(struct pt_regs *regs);
 extern unsigned long oops_begin(void);
 extern void oops_end(unsigned long, struct pt_regs *, int signr);
diff --git a/arch/x86/kernel/dumpstack.c b/arch/x86/kernel/dumpstack.c
index 9c8652974f8e..574fe5d97ba2 100644
--- a/arch/x86/kernel/dumpstack.c
+++ b/arch/x86/kernel/dumpstack.c
@@ -135,7 +135,7 @@ static void show_regs_if_on_stack(struct stack_info *info, struct pt_regs *regs,
 	 * they can be printed in the right context.
 	 */
 	if (!partial && on_stack(info, regs, sizeof(*regs))) {
-		__show_regs(regs, 0);
+		__show_regs(regs, SHOW_REGS_SHORT);
 
 	} else if (partial && on_stack(info, (void *)regs + IRET_FRAME_OFFSET,
 				       IRET_FRAME_SIZE)) {
@@ -333,7 +333,7 @@ void oops_end(unsigned long flags, struct pt_regs *regs, int signr)
 	oops_exit();
 
 	/* Executive summary in case the oops scrolled away */
-	__show_regs(&exec_summary_regs, true);
+	__show_regs(&exec_summary_regs, SHOW_REGS_ALL);
 
 	if (!signr)
 		return;
@@ -393,14 +393,9 @@ void die(const char *str, struct pt_regs *regs, long err)
 
 void show_regs(struct pt_regs *regs)
 {
-	bool all = true;
-
 	show_regs_print_info(KERN_DEFAULT);
 
-	if (IS_ENABLED(CONFIG_X86_32))
-		all = !user_mode(regs);
-
-	__show_regs(regs, all);
+	__show_regs(regs, user_mode(regs) ? SHOW_REGS_USER : SHOW_REGS_ALL);
 
 	/*
 	 * When in-kernel, we also print out the stack at the time of the fault..
diff --git a/arch/x86/kernel/process_32.c b/arch/x86/kernel/process_32.c
index 2924fd447e61..5046a3c9dec2 100644
--- a/arch/x86/kernel/process_32.c
+++ b/arch/x86/kernel/process_32.c
@@ -59,7 +59,7 @@
 #include <asm/intel_rdt_sched.h>
 #include <asm/proto.h>
 
-void __show_regs(struct pt_regs *regs, int all)
+void __show_regs(struct pt_regs *regs, enum show_regs_mode mode)
 {
 	unsigned long cr0 = 0L, cr2 = 0L, cr3 = 0L, cr4 = 0L;
 	unsigned long d0, d1, d2, d3, d6, d7;
@@ -85,7 +85,7 @@ void __show_regs(struct pt_regs *regs, int all)
 	printk(KERN_DEFAULT "DS: %04x ES: %04x FS: %04x GS: %04x SS: %04x EFLAGS: %08lx\n",
 	       (u16)regs->ds, (u16)regs->es, (u16)regs->fs, gs, ss, regs->flags);
 
-	if (!all)
+	if (mode != SHOW_REGS_ALL)
 		return;
 
 	cr0 = read_cr0();
diff --git a/arch/x86/kernel/process_64.c b/arch/x86/kernel/process_64.c
index a451bc374b9b..ea5ea850348d 100644
--- a/arch/x86/kernel/process_64.c
+++ b/arch/x86/kernel/process_64.c
@@ -62,7 +62,7 @@
 __visible DEFINE_PER_CPU(unsigned long, rsp_scratch);
 
 /* Prints also some state that isn't saved in the pt_regs */
-void __show_regs(struct pt_regs *regs, int all)
+void __show_regs(struct pt_regs *regs, enum show_regs_mode mode)
 {
 	unsigned long cr0 = 0L, cr2 = 0L, cr3 = 0L, cr4 = 0L, fs, gs, shadowgs;
 	unsigned long d0, d1, d2, d3, d6, d7;
@@ -87,9 +87,17 @@ void __show_regs(struct pt_regs *regs, int all)
 	printk(KERN_DEFAULT "R13: %016lx R14: %016lx R15: %016lx\n",
 	       regs->r13, regs->r14, regs->r15);
 
-	if (!all)
+	if (mode == SHOW_REGS_SHORT)
 		return;
 
+	if (mode == SHOW_REGS_USER) {
+		rdmsrl(MSR_FS_BASE, fs);
+		rdmsrl(MSR_KERNEL_GS_BASE, shadowgs);
+		printk(KERN_DEFAULT "FS:  %016lx GS:  %016lx\n",
+		       fs, shadowgs);
+		return;
+	}
+
 	asm("movl %%ds,%0" : "=r" (ds));
 	asm("movl %%cs,%0" : "=r" (cs));
 	asm("movl %%es,%0" : "=r" (es));
-- 
2.19.0.rc1.350.ge57e33dbd1-goog


^ permalink raw reply related	[flat|nested] 5+ messages in thread

* Re: [PATCH] x86/process: don't mix user/kernel regs in 64bit __show_regs
  2018-08-31 19:41 [PATCH] x86/process: don't mix user/kernel regs in 64bit __show_regs Jann Horn
@ 2018-08-31 20:11 ` Andy Lutomirski
  2018-09-06 11:10   ` Jann Horn
  2018-09-06 12:36 ` [tip:x86/urgent] x86/process: Don't mix user/kernel regs in 64bit __show_regs() tip-bot for Jann Horn
  1 sibling, 1 reply; 5+ messages in thread
From: Andy Lutomirski @ 2018-08-31 20:11 UTC (permalink / raw)
  To: Jann Horn
  Cc: Thomas Gleixner, Ingo Molnar, H. Peter Anvin, X86 ML,
	Andy Lutomirski, Borislav Petkov, Greg Kroah-Hartman, LKML

On Fri, Aug 31, 2018 at 12:41 PM, Jann Horn <jannh@google.com> wrote:
> When the kernel.print-fatal-signals sysctl has been enabled (I don't know
> whether anyone actually enables it), a simple userspace crash will cause
> the kernel to write a crash dump that contains, among other things, the
> kernel gsbase into dmesg.
>
> As suggested by Andy, limit output to pt_regs, FS_BASE and KERNEL_GS_BASE
> in this case.
>
> This also moves the bitness-specific logic from show_regs() into
> process_{32,64}.c.
>
> Signed-off-by: Jann Horn <jannh@google.com>
> Fixes: 45807a1df9f5 ("vdso: print fatal signals")
> ---
> @Andy: Does this look like what you had in mind?

Yes.

Although there's another option: remove print-fatal-signals.

>
> Does this need a CC stable tag? I haven't put one on it for now.

Probably.

>
>  arch/x86/include/asm/kdebug.h | 12 +++++++++++-
>  arch/x86/kernel/dumpstack.c   | 11 +++--------
>  arch/x86/kernel/process_32.c  |  4 ++--
>  arch/x86/kernel/process_64.c  | 12 ++++++++++--
>  4 files changed, 26 insertions(+), 13 deletions(-)
>
> diff --git a/arch/x86/include/asm/kdebug.h b/arch/x86/include/asm/kdebug.h
> index 395c9631e000..75f1e35e7c15 100644
> --- a/arch/x86/include/asm/kdebug.h
> +++ b/arch/x86/include/asm/kdebug.h
> @@ -22,10 +22,20 @@ enum die_val {
>         DIE_NMIUNKNOWN,
>  };
>
> +enum show_regs_mode {
> +       SHOW_REGS_SHORT,
> +       /*
> +        * For when userspace crashed, but we don't think it's our fault, and
> +        * therefore don't print kernel registers.
> +        */
> +       SHOW_REGS_USER,
> +       SHOW_REGS_ALL
> +};
> +
>  extern void die(const char *, struct pt_regs *,long);
>  extern int __must_check __die(const char *, struct pt_regs *, long);
>  extern void show_stack_regs(struct pt_regs *regs);
> -extern void __show_regs(struct pt_regs *regs, int all);
> +extern void __show_regs(struct pt_regs *regs, enum show_regs_mode);
>  extern void show_iret_regs(struct pt_regs *regs);
>  extern unsigned long oops_begin(void);
>  extern void oops_end(unsigned long, struct pt_regs *, int signr);
> diff --git a/arch/x86/kernel/dumpstack.c b/arch/x86/kernel/dumpstack.c
> index 9c8652974f8e..574fe5d97ba2 100644
> --- a/arch/x86/kernel/dumpstack.c
> +++ b/arch/x86/kernel/dumpstack.c
> @@ -135,7 +135,7 @@ static void show_regs_if_on_stack(struct stack_info *info, struct pt_regs *regs,
>          * they can be printed in the right context.
>          */
>         if (!partial && on_stack(info, regs, sizeof(*regs))) {
> -               __show_regs(regs, 0);
> +               __show_regs(regs, SHOW_REGS_SHORT);
>
>         } else if (partial && on_stack(info, (void *)regs + IRET_FRAME_OFFSET,
>                                        IRET_FRAME_SIZE)) {
> @@ -333,7 +333,7 @@ void oops_end(unsigned long flags, struct pt_regs *regs, int signr)
>         oops_exit();
>
>         /* Executive summary in case the oops scrolled away */
> -       __show_regs(&exec_summary_regs, true);
> +       __show_regs(&exec_summary_regs, SHOW_REGS_ALL);
>
>         if (!signr)
>                 return;
> @@ -393,14 +393,9 @@ void die(const char *str, struct pt_regs *regs, long err)
>
>  void show_regs(struct pt_regs *regs)
>  {
> -       bool all = true;
> -
>         show_regs_print_info(KERN_DEFAULT);
>
> -       if (IS_ENABLED(CONFIG_X86_32))
> -               all = !user_mode(regs);
> -
> -       __show_regs(regs, all);
> +       __show_regs(regs, user_mode(regs) ? SHOW_REGS_USER : SHOW_REGS_ALL);
>
>         /*
>          * When in-kernel, we also print out the stack at the time of the fault..
> diff --git a/arch/x86/kernel/process_32.c b/arch/x86/kernel/process_32.c
> index 2924fd447e61..5046a3c9dec2 100644
> --- a/arch/x86/kernel/process_32.c
> +++ b/arch/x86/kernel/process_32.c
> @@ -59,7 +59,7 @@
>  #include <asm/intel_rdt_sched.h>
>  #include <asm/proto.h>
>
> -void __show_regs(struct pt_regs *regs, int all)
> +void __show_regs(struct pt_regs *regs, enum show_regs_mode mode)
>  {
>         unsigned long cr0 = 0L, cr2 = 0L, cr3 = 0L, cr4 = 0L;
>         unsigned long d0, d1, d2, d3, d6, d7;
> @@ -85,7 +85,7 @@ void __show_regs(struct pt_regs *regs, int all)
>         printk(KERN_DEFAULT "DS: %04x ES: %04x FS: %04x GS: %04x SS: %04x EFLAGS: %08lx\n",
>                (u16)regs->ds, (u16)regs->es, (u16)regs->fs, gs, ss, regs->flags);
>
> -       if (!all)
> +       if (mode != SHOW_REGS_ALL)
>                 return;
>
>         cr0 = read_cr0();
> diff --git a/arch/x86/kernel/process_64.c b/arch/x86/kernel/process_64.c
> index a451bc374b9b..ea5ea850348d 100644
> --- a/arch/x86/kernel/process_64.c
> +++ b/arch/x86/kernel/process_64.c
> @@ -62,7 +62,7 @@
>  __visible DEFINE_PER_CPU(unsigned long, rsp_scratch);
>
>  /* Prints also some state that isn't saved in the pt_regs */
> -void __show_regs(struct pt_regs *regs, int all)
> +void __show_regs(struct pt_regs *regs, enum show_regs_mode mode)
>  {
>         unsigned long cr0 = 0L, cr2 = 0L, cr3 = 0L, cr4 = 0L, fs, gs, shadowgs;
>         unsigned long d0, d1, d2, d3, d6, d7;
> @@ -87,9 +87,17 @@ void __show_regs(struct pt_regs *regs, int all)
>         printk(KERN_DEFAULT "R13: %016lx R14: %016lx R15: %016lx\n",
>                regs->r13, regs->r14, regs->r15);
>
> -       if (!all)
> +       if (mode == SHOW_REGS_SHORT)
>                 return;
>
> +       if (mode == SHOW_REGS_USER) {
> +               rdmsrl(MSR_FS_BASE, fs);
> +               rdmsrl(MSR_KERNEL_GS_BASE, shadowgs);
> +               printk(KERN_DEFAULT "FS:  %016lx GS:  %016lx\n",
> +                      fs, shadowgs);
> +               return;
> +       }
> +
>         asm("movl %%ds,%0" : "=r" (ds));
>         asm("movl %%cs,%0" : "=r" (cs));
>         asm("movl %%es,%0" : "=r" (es));
> --
> 2.19.0.rc1.350.ge57e33dbd1-goog
>

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] x86/process: don't mix user/kernel regs in 64bit __show_regs
  2018-08-31 20:11 ` Andy Lutomirski
@ 2018-09-06 11:10   ` Jann Horn
  2018-09-06 12:31     ` Thomas Gleixner
  0 siblings, 1 reply; 5+ messages in thread
From: Jann Horn @ 2018-09-06 11:10 UTC (permalink / raw)
  To: Andy Lutomirski, Ingo Molnar
  Cc: Thomas Gleixner, H . Peter Anvin, the arch/x86 maintainers,
	bpetkov, Greg Kroah-Hartman, kernel list

On Fri, Aug 31, 2018 at 10:12 PM Andy Lutomirski <luto@kernel.org> wrote:
>
> On Fri, Aug 31, 2018 at 12:41 PM, Jann Horn <jannh@google.com> wrote:
> > When the kernel.print-fatal-signals sysctl has been enabled (I don't know
> > whether anyone actually enables it), a simple userspace crash will cause
> > the kernel to write a crash dump that contains, among other things, the
> > kernel gsbase into dmesg.
> >
> > As suggested by Andy, limit output to pt_regs, FS_BASE and KERNEL_GS_BASE
> > in this case.
> >
> > This also moves the bitness-specific logic from show_regs() into
> > process_{32,64}.c.
> >
> > Signed-off-by: Jann Horn <jannh@google.com>
> > Fixes: 45807a1df9f5 ("vdso: print fatal signals")
> > ---
> > @Andy: Does this look like what you had in mind?
>
> Yes.
>
> Although there's another option: remove print-fatal-signals.

Who wants to decide? Ingo, it's your feature - what do you think?

> > Does this need a CC stable tag? I haven't put one on it for now.
>
> Probably.
>
> >
> >  arch/x86/include/asm/kdebug.h | 12 +++++++++++-
> >  arch/x86/kernel/dumpstack.c   | 11 +++--------
> >  arch/x86/kernel/process_32.c  |  4 ++--
> >  arch/x86/kernel/process_64.c  | 12 ++++++++++--
> >  4 files changed, 26 insertions(+), 13 deletions(-)
> >
> > diff --git a/arch/x86/include/asm/kdebug.h b/arch/x86/include/asm/kdebug.h
> > index 395c9631e000..75f1e35e7c15 100644
> > --- a/arch/x86/include/asm/kdebug.h
> > +++ b/arch/x86/include/asm/kdebug.h
> > @@ -22,10 +22,20 @@ enum die_val {
> >         DIE_NMIUNKNOWN,
> >  };
> >
> > +enum show_regs_mode {
> > +       SHOW_REGS_SHORT,
> > +       /*
> > +        * For when userspace crashed, but we don't think it's our fault, and
> > +        * therefore don't print kernel registers.
> > +        */
> > +       SHOW_REGS_USER,
> > +       SHOW_REGS_ALL
> > +};
> > +
> >  extern void die(const char *, struct pt_regs *,long);
> >  extern int __must_check __die(const char *, struct pt_regs *, long);
> >  extern void show_stack_regs(struct pt_regs *regs);
> > -extern void __show_regs(struct pt_regs *regs, int all);
> > +extern void __show_regs(struct pt_regs *regs, enum show_regs_mode);
> >  extern void show_iret_regs(struct pt_regs *regs);
> >  extern unsigned long oops_begin(void);
> >  extern void oops_end(unsigned long, struct pt_regs *, int signr);
> > diff --git a/arch/x86/kernel/dumpstack.c b/arch/x86/kernel/dumpstack.c
> > index 9c8652974f8e..574fe5d97ba2 100644
> > --- a/arch/x86/kernel/dumpstack.c
> > +++ b/arch/x86/kernel/dumpstack.c
> > @@ -135,7 +135,7 @@ static void show_regs_if_on_stack(struct stack_info *info, struct pt_regs *regs,
> >          * they can be printed in the right context.
> >          */
> >         if (!partial && on_stack(info, regs, sizeof(*regs))) {
> > -               __show_regs(regs, 0);
> > +               __show_regs(regs, SHOW_REGS_SHORT);
> >
> >         } else if (partial && on_stack(info, (void *)regs + IRET_FRAME_OFFSET,
> >                                        IRET_FRAME_SIZE)) {
> > @@ -333,7 +333,7 @@ void oops_end(unsigned long flags, struct pt_regs *regs, int signr)
> >         oops_exit();
> >
> >         /* Executive summary in case the oops scrolled away */
> > -       __show_regs(&exec_summary_regs, true);
> > +       __show_regs(&exec_summary_regs, SHOW_REGS_ALL);
> >
> >         if (!signr)
> >                 return;
> > @@ -393,14 +393,9 @@ void die(const char *str, struct pt_regs *regs, long err)
> >
> >  void show_regs(struct pt_regs *regs)
> >  {
> > -       bool all = true;
> > -
> >         show_regs_print_info(KERN_DEFAULT);
> >
> > -       if (IS_ENABLED(CONFIG_X86_32))
> > -               all = !user_mode(regs);
> > -
> > -       __show_regs(regs, all);
> > +       __show_regs(regs, user_mode(regs) ? SHOW_REGS_USER : SHOW_REGS_ALL);
> >
> >         /*
> >          * When in-kernel, we also print out the stack at the time of the fault..
> > diff --git a/arch/x86/kernel/process_32.c b/arch/x86/kernel/process_32.c
> > index 2924fd447e61..5046a3c9dec2 100644
> > --- a/arch/x86/kernel/process_32.c
> > +++ b/arch/x86/kernel/process_32.c
> > @@ -59,7 +59,7 @@
> >  #include <asm/intel_rdt_sched.h>
> >  #include <asm/proto.h>
> >
> > -void __show_regs(struct pt_regs *regs, int all)
> > +void __show_regs(struct pt_regs *regs, enum show_regs_mode mode)
> >  {
> >         unsigned long cr0 = 0L, cr2 = 0L, cr3 = 0L, cr4 = 0L;
> >         unsigned long d0, d1, d2, d3, d6, d7;
> > @@ -85,7 +85,7 @@ void __show_regs(struct pt_regs *regs, int all)
> >         printk(KERN_DEFAULT "DS: %04x ES: %04x FS: %04x GS: %04x SS: %04x EFLAGS: %08lx\n",
> >                (u16)regs->ds, (u16)regs->es, (u16)regs->fs, gs, ss, regs->flags);
> >
> > -       if (!all)
> > +       if (mode != SHOW_REGS_ALL)
> >                 return;
> >
> >         cr0 = read_cr0();
> > diff --git a/arch/x86/kernel/process_64.c b/arch/x86/kernel/process_64.c
> > index a451bc374b9b..ea5ea850348d 100644
> > --- a/arch/x86/kernel/process_64.c
> > +++ b/arch/x86/kernel/process_64.c
> > @@ -62,7 +62,7 @@
> >  __visible DEFINE_PER_CPU(unsigned long, rsp_scratch);
> >
> >  /* Prints also some state that isn't saved in the pt_regs */
> > -void __show_regs(struct pt_regs *regs, int all)
> > +void __show_regs(struct pt_regs *regs, enum show_regs_mode mode)
> >  {
> >         unsigned long cr0 = 0L, cr2 = 0L, cr3 = 0L, cr4 = 0L, fs, gs, shadowgs;
> >         unsigned long d0, d1, d2, d3, d6, d7;
> > @@ -87,9 +87,17 @@ void __show_regs(struct pt_regs *regs, int all)
> >         printk(KERN_DEFAULT "R13: %016lx R14: %016lx R15: %016lx\n",
> >                regs->r13, regs->r14, regs->r15);
> >
> > -       if (!all)
> > +       if (mode == SHOW_REGS_SHORT)
> >                 return;
> >
> > +       if (mode == SHOW_REGS_USER) {
> > +               rdmsrl(MSR_FS_BASE, fs);
> > +               rdmsrl(MSR_KERNEL_GS_BASE, shadowgs);
> > +               printk(KERN_DEFAULT "FS:  %016lx GS:  %016lx\n",
> > +                      fs, shadowgs);
> > +               return;
> > +       }
> > +
> >         asm("movl %%ds,%0" : "=r" (ds));
> >         asm("movl %%cs,%0" : "=r" (cs));
> >         asm("movl %%es,%0" : "=r" (es));
> > --
> > 2.19.0.rc1.350.ge57e33dbd1-goog
> >

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] x86/process: don't mix user/kernel regs in 64bit __show_regs
  2018-09-06 11:10   ` Jann Horn
@ 2018-09-06 12:31     ` Thomas Gleixner
  0 siblings, 0 replies; 5+ messages in thread
From: Thomas Gleixner @ 2018-09-06 12:31 UTC (permalink / raw)
  To: Jann Horn
  Cc: Andy Lutomirski, Ingo Molnar, H . Peter Anvin,
	the arch/x86 maintainers, bpetkov, Greg Kroah-Hartman,
	kernel list

On Thu, 6 Sep 2018, Jann Horn wrote:
> On Fri, Aug 31, 2018 at 10:12 PM Andy Lutomirski <luto@kernel.org> wrote:
> >
> > On Fri, Aug 31, 2018 at 12:41 PM, Jann Horn <jannh@google.com> wrote:
> > > When the kernel.print-fatal-signals sysctl has been enabled (I don't know
> > > whether anyone actually enables it), a simple userspace crash will cause
> > > the kernel to write a crash dump that contains, among other things, the
> > > kernel gsbase into dmesg.
> > >
> > > As suggested by Andy, limit output to pt_regs, FS_BASE and KERNEL_GS_BASE
> > > in this case.
> > >
> > > This also moves the bitness-specific logic from show_regs() into
> > > process_{32,64}.c.
> > >
> > > Signed-off-by: Jann Horn <jannh@google.com>
> > > Fixes: 45807a1df9f5 ("vdso: print fatal signals")
> > > ---
> > > @Andy: Does this look like what you had in mind?
> >
> > Yes.
> >
> > Although there's another option: remove print-fatal-signals.
> 
> Who wants to decide? Ingo, it's your feature - what do you think?

It seems to be documented for trouble shooting in lots of places and the
fix is not horrible. So lets keep it.

Thanks,

	tglx

^ permalink raw reply	[flat|nested] 5+ messages in thread

* [tip:x86/urgent] x86/process: Don't mix user/kernel regs in 64bit __show_regs()
  2018-08-31 19:41 [PATCH] x86/process: don't mix user/kernel regs in 64bit __show_regs Jann Horn
  2018-08-31 20:11 ` Andy Lutomirski
@ 2018-09-06 12:36 ` tip-bot for Jann Horn
  1 sibling, 0 replies; 5+ messages in thread
From: tip-bot for Jann Horn @ 2018-09-06 12:36 UTC (permalink / raw)
  To: linux-tip-commits
  Cc: jannh, hpa, mingo, tglx, bpetkov, luto, linux-kernel, gregkh

Commit-ID:  9fe6299dde587788f245e9f7a5a1b296fad4e8c7
Gitweb:     https://git.kernel.org/tip/9fe6299dde587788f245e9f7a5a1b296fad4e8c7
Author:     Jann Horn <jannh@google.com>
AuthorDate: Fri, 31 Aug 2018 21:41:51 +0200
Committer:  Thomas Gleixner <tglx@linutronix.de>
CommitDate: Thu, 6 Sep 2018 14:33:12 +0200

x86/process: Don't mix user/kernel regs in 64bit __show_regs()

When the kernel.print-fatal-signals sysctl has been enabled, a simple
userspace crash will cause the kernel to write a crash dump that contains,
among other things, the kernel gsbase into dmesg.

As suggested by Andy, limit output to pt_regs, FS_BASE and KERNEL_GS_BASE
in this case.

This also moves the bitness-specific logic from show_regs() into
process_{32,64}.c.

Fixes: 45807a1df9f5 ("vdso: print fatal signals")
Signed-off-by: Jann Horn <jannh@google.com>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Cc: "H. Peter Anvin" <hpa@zytor.com>
Cc: Andy Lutomirski <luto@kernel.org>
Cc: Borislav Petkov <bpetkov@suse.de>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: stable@vger.kernel.org
Link: https://lkml.kernel.org/r/20180831194151.123586-1-jannh@google.com

---
 arch/x86/include/asm/kdebug.h | 12 +++++++++++-
 arch/x86/kernel/dumpstack.c   | 11 +++--------
 arch/x86/kernel/process_32.c  |  4 ++--
 arch/x86/kernel/process_64.c  | 12 ++++++++++--
 4 files changed, 26 insertions(+), 13 deletions(-)

diff --git a/arch/x86/include/asm/kdebug.h b/arch/x86/include/asm/kdebug.h
index 395c9631e000..75f1e35e7c15 100644
--- a/arch/x86/include/asm/kdebug.h
+++ b/arch/x86/include/asm/kdebug.h
@@ -22,10 +22,20 @@ enum die_val {
 	DIE_NMIUNKNOWN,
 };
 
+enum show_regs_mode {
+	SHOW_REGS_SHORT,
+	/*
+	 * For when userspace crashed, but we don't think it's our fault, and
+	 * therefore don't print kernel registers.
+	 */
+	SHOW_REGS_USER,
+	SHOW_REGS_ALL
+};
+
 extern void die(const char *, struct pt_regs *,long);
 extern int __must_check __die(const char *, struct pt_regs *, long);
 extern void show_stack_regs(struct pt_regs *regs);
-extern void __show_regs(struct pt_regs *regs, int all);
+extern void __show_regs(struct pt_regs *regs, enum show_regs_mode);
 extern void show_iret_regs(struct pt_regs *regs);
 extern unsigned long oops_begin(void);
 extern void oops_end(unsigned long, struct pt_regs *, int signr);
diff --git a/arch/x86/kernel/dumpstack.c b/arch/x86/kernel/dumpstack.c
index f56895106ccf..2b5886401e5f 100644
--- a/arch/x86/kernel/dumpstack.c
+++ b/arch/x86/kernel/dumpstack.c
@@ -146,7 +146,7 @@ static void show_regs_if_on_stack(struct stack_info *info, struct pt_regs *regs,
 	 * they can be printed in the right context.
 	 */
 	if (!partial && on_stack(info, regs, sizeof(*regs))) {
-		__show_regs(regs, 0);
+		__show_regs(regs, SHOW_REGS_SHORT);
 
 	} else if (partial && on_stack(info, (void *)regs + IRET_FRAME_OFFSET,
 				       IRET_FRAME_SIZE)) {
@@ -344,7 +344,7 @@ void oops_end(unsigned long flags, struct pt_regs *regs, int signr)
 	oops_exit();
 
 	/* Executive summary in case the oops scrolled away */
-	__show_regs(&exec_summary_regs, true);
+	__show_regs(&exec_summary_regs, SHOW_REGS_ALL);
 
 	if (!signr)
 		return;
@@ -407,14 +407,9 @@ void die(const char *str, struct pt_regs *regs, long err)
 
 void show_regs(struct pt_regs *regs)
 {
-	bool all = true;
-
 	show_regs_print_info(KERN_DEFAULT);
 
-	if (IS_ENABLED(CONFIG_X86_32))
-		all = !user_mode(regs);
-
-	__show_regs(regs, all);
+	__show_regs(regs, user_mode(regs) ? SHOW_REGS_USER : SHOW_REGS_ALL);
 
 	/*
 	 * When in-kernel, we also print out the stack at the time of the fault..
diff --git a/arch/x86/kernel/process_32.c b/arch/x86/kernel/process_32.c
index 2924fd447e61..5046a3c9dec2 100644
--- a/arch/x86/kernel/process_32.c
+++ b/arch/x86/kernel/process_32.c
@@ -59,7 +59,7 @@
 #include <asm/intel_rdt_sched.h>
 #include <asm/proto.h>
 
-void __show_regs(struct pt_regs *regs, int all)
+void __show_regs(struct pt_regs *regs, enum show_regs_mode mode)
 {
 	unsigned long cr0 = 0L, cr2 = 0L, cr3 = 0L, cr4 = 0L;
 	unsigned long d0, d1, d2, d3, d6, d7;
@@ -85,7 +85,7 @@ void __show_regs(struct pt_regs *regs, int all)
 	printk(KERN_DEFAULT "DS: %04x ES: %04x FS: %04x GS: %04x SS: %04x EFLAGS: %08lx\n",
 	       (u16)regs->ds, (u16)regs->es, (u16)regs->fs, gs, ss, regs->flags);
 
-	if (!all)
+	if (mode != SHOW_REGS_ALL)
 		return;
 
 	cr0 = read_cr0();
diff --git a/arch/x86/kernel/process_64.c b/arch/x86/kernel/process_64.c
index a451bc374b9b..ea5ea850348d 100644
--- a/arch/x86/kernel/process_64.c
+++ b/arch/x86/kernel/process_64.c
@@ -62,7 +62,7 @@
 __visible DEFINE_PER_CPU(unsigned long, rsp_scratch);
 
 /* Prints also some state that isn't saved in the pt_regs */
-void __show_regs(struct pt_regs *regs, int all)
+void __show_regs(struct pt_regs *regs, enum show_regs_mode mode)
 {
 	unsigned long cr0 = 0L, cr2 = 0L, cr3 = 0L, cr4 = 0L, fs, gs, shadowgs;
 	unsigned long d0, d1, d2, d3, d6, d7;
@@ -87,9 +87,17 @@ void __show_regs(struct pt_regs *regs, int all)
 	printk(KERN_DEFAULT "R13: %016lx R14: %016lx R15: %016lx\n",
 	       regs->r13, regs->r14, regs->r15);
 
-	if (!all)
+	if (mode == SHOW_REGS_SHORT)
 		return;
 
+	if (mode == SHOW_REGS_USER) {
+		rdmsrl(MSR_FS_BASE, fs);
+		rdmsrl(MSR_KERNEL_GS_BASE, shadowgs);
+		printk(KERN_DEFAULT "FS:  %016lx GS:  %016lx\n",
+		       fs, shadowgs);
+		return;
+	}
+
 	asm("movl %%ds,%0" : "=r" (ds));
 	asm("movl %%cs,%0" : "=r" (cs));
 	asm("movl %%es,%0" : "=r" (es));

^ permalink raw reply related	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2018-09-06 12:37 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-08-31 19:41 [PATCH] x86/process: don't mix user/kernel regs in 64bit __show_regs Jann Horn
2018-08-31 20:11 ` Andy Lutomirski
2018-09-06 11:10   ` Jann Horn
2018-09-06 12:31     ` Thomas Gleixner
2018-09-06 12:36 ` [tip:x86/urgent] x86/process: Don't mix user/kernel regs in 64bit __show_regs() tip-bot for Jann Horn

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.