| Message ID | 5CF0F1410200007800233D67@prv1-mh.provo.novell.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | : x86/traps: improve show_trace()'s top-of-stack handling | expand |
On 31/05/2019 10:17, Jan Beulich wrote: > Nothing (afaics) guarantees that the original frame's stack pointer > points at readable memory. Having hit just the scenario described here, the answer is "nothing". > Avoid a (likely nested) crash by attaching > exception recovery to the read (making it a single read at the same > time). Don't even invoke _show_trace() in case of a non-readable top > slot. > > Signed-off-by: Jan Beulich <jbeulich@suse.com> > > --- a/xen/arch/x86/traps.c > +++ b/xen/arch/x86/traps.c > @@ -484,16 +484,23 @@ static void _show_trace(unsigned long sp > > static void show_trace(const struct cpu_user_regs *regs) > { > - unsigned long *sp = ESP_BEFORE_EXCEPTION(regs); > + unsigned long *sp = ESP_BEFORE_EXCEPTION(regs), tos = 0; > > printk("Xen call trace:\n"); > /* Probe the stack for readability. */ > + asm ( "1: mov %2,%0; 2:\n" > + ".pushsection .fixup,\"ax\"\n" > + "3: xor %k1,%k1; jmp 2b\n" Can we use some named parameters, so the asm can actually be followed? Also, you can't do this by zeroing sp, because it aliases with "sp was at zero and readable". A better option would be to get an explicit fault boolean out of the asm. > + ".popsection\n" > + _ASM_EXTABLE(1b, 3b) > + : "+r" (tos), "+r" (sp) : "m" (*sp) ); > + > /* > * If RIP looks sensible, or the top of the stack doesn't, print RIP at > * the top of the stack trace. > */ > if ( is_active_kernel_text(regs->rip) || > - !is_active_kernel_text(*sp) ) > + !is_active_kernel_text(tos) ) > printk(" [<%p>] %pS\n", _p(regs->rip), _p(regs->rip)); > /* > * Else RIP looks bad but the top of the stack looks good. Perhaps we > @@ -501,12 +508,15 @@ static void show_trace(const struct cpu_ > * return address; print it and skip past so _show_trace() doesn't print > * it again. > */ > - else > + else if ( sp ) > { > - printk(" [<%p>] %pS\n", _p(*sp), _p(*sp)); > + printk(" [<%p>] %pS\n", _p(tos), _p(tos)); > sp++; > } > > + if ( !sp ) > + return; Along with the alias mentioned above, this also has a boundary case when sp is -8, due to the sp++ above. It would probably be better to fit an else if ( fault ) { printk(" [Fault on access]\n"); return; } into the middle of the existing if/else. ~Andrew
>>> On 07.06.19 at 19:51, <andrew.cooper3@citrix.com> wrote: > On 31/05/2019 10:17, Jan Beulich wrote: >> --- a/xen/arch/x86/traps.c >> +++ b/xen/arch/x86/traps.c >> @@ -484,16 +484,23 @@ static void _show_trace(unsigned long sp >> >> static void show_trace(const struct cpu_user_regs *regs) >> { >> - unsigned long *sp = ESP_BEFORE_EXCEPTION(regs); >> + unsigned long *sp = ESP_BEFORE_EXCEPTION(regs), tos = 0; >> >> printk("Xen call trace:\n"); >> > > /* Probe the stack for readability. */ That's not an appropriate comment for this code fragment, at least not with my (non-native) understanding of "probe". To me the verb does not include reading actual data, yet that's what we do here. If anything is needed at all, then maybe "Guarded read of the stack top"? >> + asm ( "1: mov %2,%0; 2:\n" >> + ".pushsection .fixup,\"ax\"\n" >> + "3: xor %k1,%k1; jmp 2b\n" > > Can we use some named parameters, so the asm can actually be followed? Sure. I did consider doing so, but then thought the one here would be simple enough. > Also, you can't do this by zeroing sp, because it aliases with "sp was > at zero and readable". A better option would be to get an explicit > fault boolean out of the asm. Hmm, this was actually deliberate: A zero %rsp is a clear sign of the stack being bad, and better not getting dumped from. >> @@ -501,12 +508,15 @@ static void show_trace(const struct cpu_ >> * return address; print it and skip past so _show_trace() doesn't print >> * it again. >> */ >> - else >> + else if ( sp ) >> { >> - printk(" [<%p>] %pS\n", _p(*sp), _p(*sp)); >> + printk(" [<%p>] %pS\n", _p(tos), _p(tos)); >> sp++; >> } >> >> + if ( !sp ) >> + return; > > Along with the alias mentioned above, this also has a boundary case when > sp is -8, due to the sp++ above. Hmm, yes, until the next patch. > It would probably be better to fit an > > else if ( fault ) > { > printk(" [Fault on access]\n"); > return; > } > > into the middle of the existing if/else. Well, okay, I'll add such a separate boolean then. I wanted to avoid further hampering readability of the asm()... Jan
--- a/xen/arch/x86/traps.c +++ b/xen/arch/x86/traps.c @@ -484,16 +484,23 @@ static void _show_trace(unsigned long sp static void show_trace(const struct cpu_user_regs *regs) { - unsigned long *sp = ESP_BEFORE_EXCEPTION(regs); + unsigned long *sp = ESP_BEFORE_EXCEPTION(regs), tos = 0; printk("Xen call trace:\n"); + asm ( "1: mov %2,%0; 2:\n" + ".pushsection .fixup,\"ax\"\n" + "3: xor %k1,%k1; jmp 2b\n" + ".popsection\n" + _ASM_EXTABLE(1b, 3b) + : "+r" (tos), "+r" (sp) : "m" (*sp) ); + /* * If RIP looks sensible, or the top of the stack doesn't, print RIP at * the top of the stack trace. */ if ( is_active_kernel_text(regs->rip) || - !is_active_kernel_text(*sp) ) + !is_active_kernel_text(tos) ) printk(" [<%p>] %pS\n", _p(regs->rip), _p(regs->rip)); /* * Else RIP looks bad but the top of the stack looks good. Perhaps we @@ -501,12 +508,15 @@ static void show_trace(const struct cpu_ * return address; print it and skip past so _show_trace() doesn't print * it again. */ - else + else if ( sp ) { - printk(" [<%p>] %pS\n", _p(*sp), _p(*sp)); + printk(" [<%p>] %pS\n", _p(tos), _p(tos)); sp++; } + if ( !sp ) + return; + _show_trace((unsigned long)sp, regs->rbp); printk("\n");
Nothing (afaics) guarantees that the original frame's stack pointer points at readable memory. Avoid a (likely nested) crash by attaching exception recovery to the read (making it a single read at the same time). Don't even invoke _show_trace() in case of a non-readable top slot. Signed-off-by: Jan Beulich <jbeulich@suse.com>