Re: [Xen-devel] [PATCH v2] x86/emul: Poision the stubs with debug traps
>>> On 30.03.17 at 19:09, wrote: > If you do have an external debugger attached, you don't need embedded > int3's to start with, and you'd still hit the debugger_trap_*() calls > before dying. Well, my experience (on other OSes with a better debugger) is different - there are occasions where it is easier to embed an int3 right away than to break into the system (doing of which may be a problem of its own if you need the breakpoint quite early during boot), go hunt for the precise instruction you want the breakpoint on, and then set the intended breakpoint using debugger facilities. > If you don't have an external debugger attached, ignoring int3's is very > poor behaviour. Well, that's your opinion. Mine (again based on past experience) is different, and it appears to match that of whoever originally wrote things this way. Nevertheless I appreciate the (assumed) reasoning behind your way of thinking. Jan ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v2] x86/emul: Poision the stubs with debug traps
On 30/03/17 13:08, Jan Beulich wrote: On 30.03.17 at 13:56, wrote: >> On 20/03/17 11:50, Jan Beulich wrote: >> On 20.03.17 at 11:58, wrote: ...rather than leaving fragments of old instructions in place. This reduces the chances of something going further-wrong (as the debug trap will be caught and terminate the guest) in a cascade-failure where we end up executing the instruction fragments. Before: (XEN) d2v0 exception 6 (ec=) in emulation stub (line 6239) (XEN) d2v0 stub: c4 e1 44 77 c3 80 d0 82 ff ff ff d1 90 ec 90 After: (XEN) d3v0 exception 6 (ec=) in emulation stub (line 6239) (XEN) d3v0 stub: c4 e1 44 77 c3 cc cc cc cc cc cc cc cc cc cc To make this work, the int3 handler needs to be extended to attempt recovery rather than simply returning back to Xen context. This is a good general precaution anyway, as nothing good will happen from letting Xen blindly execute over 0xcc's. >>> I partly disagree: It can be a very useful thing to not have to rebuild >>> Xen with hard coded INT3 invocations removed/re-added between >>> runs with or without a debugger attached. >> What possible usecase is this for? If you don't already modify >> do_int3(), use of embedded int3's are of no use. > How are they not, with an external debugger attached? (I admit > that I have no idea in what good or bad a state the external > debugger support code is that we have.) I have never succeeded at using the external debugger facilities, but I have only tried once or twice. If you do have an external debugger attached, you don't need embedded int3's to start with, and you'd still hit the debugger_trap_*() calls before dying. If you don't have an external debugger attached, ignoring int3's is very poor behaviour. The fact that it ever worked is a bug IMO. ~Andrew ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v2] x86/emul: Poision the stubs with debug traps
>>> On 30.03.17 at 13:56, wrote: > On 20/03/17 11:50, Jan Beulich wrote: > On 20.03.17 at 11:58, wrote: >>> ...rather than leaving fragments of old instructions in place. This reduces >>> the chances of something going further-wrong (as the debug trap will be >>> caught >>> and terminate the guest) in a cascade-failure where we end up executing the >>> instruction fragments. >>> >>> Before: >>> (XEN) d2v0 exception 6 (ec=) in emulation stub (line 6239) >>> (XEN) d2v0 stub: c4 e1 44 77 c3 80 d0 82 ff ff ff d1 90 ec 90 >>> >>> After: >>> (XEN) d3v0 exception 6 (ec=) in emulation stub (line 6239) >>> (XEN) d3v0 stub: c4 e1 44 77 c3 cc cc cc cc cc cc cc cc cc cc >>> >>> To make this work, the int3 handler needs to be extended to attempt recovery >>> rather than simply returning back to Xen context. This is a good general >>> precaution anyway, as nothing good will happen from letting Xen blindly >>> execute over 0xcc's. >> I partly disagree: It can be a very useful thing to not have to rebuild >> Xen with hard coded INT3 invocations removed/re-added between >> runs with or without a debugger attached. > > What possible usecase is this for? If you don't already modify > do_int3(), use of embedded int3's are of no use. How are they not, with an external debugger attached? (I admit that I have no idea in what good or bad a state the external debugger support code is that we have.) Jan ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v2] x86/emul: Poision the stubs with debug traps
On 20/03/17 11:50, Jan Beulich wrote:
On 20.03.17 at 11:58, wrote:
>> ...rather than leaving fragments of old instructions in place. This reduces
>> the chances of something going further-wrong (as the debug trap will be
>> caught
>> and terminate the guest) in a cascade-failure where we end up executing the
>> instruction fragments.
>>
>> Before:
>> (XEN) d2v0 exception 6 (ec=) in emulation stub (line 6239)
>> (XEN) d2v0 stub: c4 e1 44 77 c3 80 d0 82 ff ff ff d1 90 ec 90
>>
>> After:
>> (XEN) d3v0 exception 6 (ec=) in emulation stub (line 6239)
>> (XEN) d3v0 stub: c4 e1 44 77 c3 cc cc cc cc cc cc cc cc cc cc
>>
>> To make this work, the int3 handler needs to be extended to attempt recovery
>> rather than simply returning back to Xen context. This is a good general
>> precaution anyway, as nothing good will happen from letting Xen blindly
>> execute over 0xcc's.
> I partly disagree: It can be a very useful thing to not have to rebuild
> Xen with hard coded INT3 invocations removed/re-added between
> runs with or without a debugger attached.
What possible usecase is this for? If you don't already modify
do_int3(), use of embedded int3's are of no use.
>
>> --- a/xen/arch/x86/traps.c
>> +++ b/xen/arch/x86/traps.c
>> @@ -1206,9 +1206,21 @@ void do_int3(struct cpu_user_regs *regs)
>>
>> if ( !guest_mode(regs) )
>> {
>> -debugger_trap_fatal(TRAP_int3, regs);
>> -return;
>> -}
>> +unsigned long fixup;
>> +
>> +if ( (fixup = search_exception_table(regs)) != 0 )
>> +{
>> +this_cpu(last_extable_addr) = regs->rip;
>> +regs->rip = fixup;
> Should this be accompanied by a dprintk(), like most other such
> cases are?
Perhaps. I'm not fussed, and it should be a rare occurrence.
>
>> +return;
>> +}
>> +
>> +if ( debugger_trap_fatal(TRAP_int3, regs) )
>> +return;
>> +
>> +show_execution_state(regs);
>> +panic("FATAL TRAP: vector = %d (int3)", TRAP_int3);
> Along the lines of what I've said above, I'd like to see this being made
> conditional upon NDEBUG.
Without some further explanation, I don't buy it as a plausible scenario.
> Plus I think you want to call fatal_trap() here.
fatal_trap() however would be better.
~Andrew
___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v2] x86/emul: Poision the stubs with debug traps
>>> On 20.03.17 at 11:58, wrote:
> ...rather than leaving fragments of old instructions in place. This reduces
> the chances of something going further-wrong (as the debug trap will be
> caught
> and terminate the guest) in a cascade-failure where we end up executing the
> instruction fragments.
>
> Before:
> (XEN) d2v0 exception 6 (ec=) in emulation stub (line 6239)
> (XEN) d2v0 stub: c4 e1 44 77 c3 80 d0 82 ff ff ff d1 90 ec 90
>
> After:
> (XEN) d3v0 exception 6 (ec=) in emulation stub (line 6239)
> (XEN) d3v0 stub: c4 e1 44 77 c3 cc cc cc cc cc cc cc cc cc cc
>
> To make this work, the int3 handler needs to be extended to attempt recovery
> rather than simply returning back to Xen context. This is a good general
> precaution anyway, as nothing good will happen from letting Xen blindly
> execute over 0xcc's.
I partly disagree: It can be a very useful thing to not have to rebuild
Xen with hard coded INT3 invocations removed/re-added between
runs with or without a debugger attached.
> --- a/xen/arch/x86/traps.c
> +++ b/xen/arch/x86/traps.c
> @@ -1206,9 +1206,21 @@ void do_int3(struct cpu_user_regs *regs)
>
> if ( !guest_mode(regs) )
> {
> -debugger_trap_fatal(TRAP_int3, regs);
> -return;
> -}
> +unsigned long fixup;
> +
> +if ( (fixup = search_exception_table(regs)) != 0 )
> +{
> +this_cpu(last_extable_addr) = regs->rip;
> +regs->rip = fixup;
Should this be accompanied by a dprintk(), like most other such
cases are?
> +return;
> +}
> +
> +if ( debugger_trap_fatal(TRAP_int3, regs) )
> +return;
> +
> +show_execution_state(regs);
> +panic("FATAL TRAP: vector = %d (int3)", TRAP_int3);
Along the lines of what I've said above, I'd like to see this being made
conditional upon NDEBUG. Plus I think you want to call fatal_trap()
here.
Jan
___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
[Xen-devel] [PATCH v2] x86/emul: Poision the stubs with debug traps
...rather than leaving fragments of old instructions in place. This reduces
the chances of something going further-wrong (as the debug trap will be caught
and terminate the guest) in a cascade-failure where we end up executing the
instruction fragments.
Before:
(XEN) d2v0 exception 6 (ec=) in emulation stub (line 6239)
(XEN) d2v0 stub: c4 e1 44 77 c3 80 d0 82 ff ff ff d1 90 ec 90
After:
(XEN) d3v0 exception 6 (ec=) in emulation stub (line 6239)
(XEN) d3v0 stub: c4 e1 44 77 c3 cc cc cc cc cc cc cc cc cc cc
To make this work, the int3 handler needs to be extended to attempt recovery
rather than simply returning back to Xen context. This is a good general
precaution anyway, as nothing good will happen from letting Xen blindly
execute over 0xcc's.
Extend the selftests to include int3, and add an extra printk indicating the
start of the recovery selftests, to avoid leaving two otherwise-spurious
faults visible in the log.
(XEN) build-id: f0a0617cdb725551d03689c23bc59f34d329c182
(XEN) Running stub recovery selftests...
(XEN) traps.c:3463: GPF (): 82d0b041 [82d0b041] ->
82d08025789a
(XEN) traps.c:813: Trap 12: 82d0b040 [82d0b040] ->
82d08025789a
(XEN) ACPI sleep modes: S3
Signed-off-by: Andrew Cooper
---
CC: Jan Beulich
v2:
* Add selftest. Recover from int3.
---
xen/arch/x86/extable.c | 4
xen/arch/x86/traps.c | 18 +++---
xen/arch/x86/x86_emulate.c | 4 ++--
3 files changed, 21 insertions(+), 5 deletions(-)
diff --git a/xen/arch/x86/extable.c b/xen/arch/x86/extable.c
index 03af2c9..6fffe05 100644
--- a/xen/arch/x86/extable.c
+++ b/xen/arch/x86/extable.c
@@ -140,10 +140,14 @@ static int __init stub_selftest(void)
{ .opc = { 0x02, 0x04, 0x04, 0xc3 }, /* add (%rsp,%rax),%al */
.rax = 0xfedcba9876543210,
.res.fields.trapnr = TRAP_stack_error },
+{ .opc = { 0xcc, 0xc3, 0xc3, 0xc3 }, /* int3 */
+ .res.fields.trapnr = TRAP_int3 },
};
unsigned long addr = this_cpu(stubs.addr) + STUB_BUF_SIZE / 2;
unsigned int i;
+printk("Running stub recovery selftests...\n");
+
for ( i = 0; i < ARRAY_SIZE(tests); ++i )
{
uint8_t *ptr = map_domain_page(_mfn(this_cpu(stubs.mfn))) +
diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c
index 0d54baf..fd5fb56 100644
--- a/xen/arch/x86/traps.c
+++ b/xen/arch/x86/traps.c
@@ -1206,9 +1206,21 @@ void do_int3(struct cpu_user_regs *regs)
if ( !guest_mode(regs) )
{
-debugger_trap_fatal(TRAP_int3, regs);
-return;
-}
+unsigned long fixup;
+
+if ( (fixup = search_exception_table(regs)) != 0 )
+{
+this_cpu(last_extable_addr) = regs->rip;
+regs->rip = fixup;
+return;
+}
+
+if ( debugger_trap_fatal(TRAP_int3, regs) )
+return;
+
+show_execution_state(regs);
+panic("FATAL TRAP: vector = %d (int3)", TRAP_int3);
+}
do_guest_trap(TRAP_int3, regs);
}
diff --git a/xen/arch/x86/x86_emulate.c b/xen/arch/x86/x86_emulate.c
index 51df340..cc334ca 100644
--- a/xen/arch/x86/x86_emulate.c
+++ b/xen/arch/x86/x86_emulate.c
@@ -30,8 +30,8 @@
BUILD_BUG_ON(STUB_BUF_SIZE / 2 < MAX_INST_LEN + 1); \
ASSERT(!(stb).ptr); \
(stb).addr = this_cpu(stubs.addr) + STUB_BUF_SIZE / 2; \
-((stb).ptr = map_domain_page(_mfn(this_cpu(stubs.mfn + \
-((stb).addr & ~PAGE_MASK); \
+memset(((stb).ptr = map_domain_page(_mfn(this_cpu(stubs.mfn + \
+ ((stb).addr & ~PAGE_MASK), 0xcc, STUB_BUF_SIZE / 2);\
})
#define put_stub(stb) ({ \
if ( (stb).ptr ) \
--
2.1.4
___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
