1: guard top-of-stack reads 2: widen condition for logging top-of-stack The issue patch 2 fixes (a curious lack of an intermediate call stack entry) was observed in practice; patch 1 is a result of me just looking at the code (and if I have missed some aspect of why this isn't a problem in reality, that patch could be easily dropped). Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
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>
--- 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");
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
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
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
>>> 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
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Despite -fno-omit-frame-pointer the compiler may omit the frame pointer,
often for relatively simple leaf functions. (To give a specific example,
the case I've run into this with is _pci_hide_device() and gcc 8.
Interestingly the even more simple neighboring iommu_has_feature() does
get a frame pointer set up, around just a single instruction. But this
may be a result of the size-of-asm() effects discussed elsewhere.)
Log the top-of-stack value if it looks valid _or_ if RIP looks invalid.
Also annotate non-frame-pointer-based stack trace entries with a
question mark, to signal clearly that any one of them may not actually
be part of the call stack.
Signed-off-by: Jan Beulich <jbeulich@suse.com>
--- a/xen/arch/x86/traps.c
+++ b/xen/arch/x86/traps.c
@@ -431,7 +431,7 @@ static void _show_trace(unsigned long sp
{
addr = *stack++;
if ( is_active_kernel_text(addr) )
- printk(" [<%p>] %pS\n", _p(addr), _p(addr));
+ printk(" [<%p>] ? %pS\n", _p(addr), _p(addr));
}
}
@@ -502,21 +502,25 @@ static void show_trace(const struct cpu_
if ( is_active_kernel_text(regs->rip) ||
!is_active_kernel_text(tos) )
printk(" [<%p>] %pS\n", _p(regs->rip), _p(regs->rip));
+
+ if ( !sp )
+ return;
+
/*
- * Else RIP looks bad but the top of the stack looks good. Perhaps we
- * followed a wild function pointer? Lets assume the top of the stack is a
+ * If RIP looks bad or the top of the stack looks good, log the top of
+ * stack as well. Perhaps we followed a wild function pointer, or we're
+ * in a function without frame pointer, or in a function prologue before
+ * the frame pointer gets set up? Lets assume the top of the stack is a
* return address; print it and skip past so _show_trace() doesn't print
* it again.
*/
- else if ( sp )
+ if ( !is_active_kernel_text(regs->rip) ||
+ is_active_kernel_text(tos) )
{
- printk(" [<%p>] %pS\n", _p(tos), _p(tos));
+ printk(" [<%p>] ? %pS\n", _p(tos), _p(tos));
sp++;
}
- if ( !sp )
- return;
-
_show_trace((unsigned long)sp, regs->rbp);
printk("\n");
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
On 31/05/2019 10:22, Jan Beulich wrote: > Despite -fno-omit-frame-pointer the compiler may omit the frame pointer, > often for relatively simple leaf functions. (To give a specific example, > the case I've run into this with is _pci_hide_device() and gcc 8. > Interestingly the even more simple neighboring iommu_has_feature() does > get a frame pointer set up, around just a single instruction. But this > may be a result of the size-of-asm() effects discussed elsewhere.) > > Log the top-of-stack value if it looks valid _or_ if RIP looks invalid. > > Also annotate non-frame-pointer-based stack trace entries with a > question mark, to signal clearly that any one of them may not actually > be part of the call stack. I very specifically didn't do that before, because it give the false impression that when a question mark isn't present, the logging line is accurate. This is not true for %rbp corruption, asm blocks which don't respect the frame pointer ABI (arguably also corruption), any fault raised from within an EFI call. Porting Xen to use objtool would be a definite improvement, but cannot guard against unexpected %rbp corruption and the EFI case. ~Andrew _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
>>> On 07.06.19 at 20:01, <andrew.cooper3@citrix.com> wrote: > On 31/05/2019 10:22, Jan Beulich wrote: >> Despite -fno-omit-frame-pointer the compiler may omit the frame pointer, >> often for relatively simple leaf functions. (To give a specific example, >> the case I've run into this with is _pci_hide_device() and gcc 8. >> Interestingly the even more simple neighboring iommu_has_feature() does >> get a frame pointer set up, around just a single instruction. But this >> may be a result of the size-of-asm() effects discussed elsewhere.) >> >> Log the top-of-stack value if it looks valid _or_ if RIP looks invalid. >> >> Also annotate non-frame-pointer-based stack trace entries with a >> question mark, to signal clearly that any one of them may not actually >> be part of the call stack. > > I very specifically didn't do that before, because it give the false > impression that when a question mark isn't present, the logging line is > accurate. > > This is not true for %rbp corruption, asm blocks which don't respect the > frame pointer ABI (arguably also corruption), any fault raised from > within an EFI call. So what do you suggest instead? Somehow we should mark slots that are more guesses than actually derived. > Porting Xen to use objtool would be a definite improvement, but cannot > guard against unexpected %rbp corruption and the EFI case. I'm not sure about "definite", but I think I see your point. Personally I continue to believe that programmer (assembly code) and compiler (C code) attached unwind annotations are the better model. Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
© 2016 - 2024 Red Hat, Inc.