Re: [Xen-devel] [PATCH for-4.12] x86/svm: Fix handling of ICEBP intercepts
On 2/1/19 8:49 AM, Andrew Cooper wrote:
> c/s 9338a37d "x86/svm: implement debug events" added support for introspecting
> ICEBP debug exceptions, but didn't account for the fact that
> svm_get_insn_len() (previously __get_instruction_length) can fail and may
> already raise #GP for the guest.
>
> If svm_get_insn_len() fails, return back to guest context rather than
> continuing and mistaking a trap-style VMExit for a fault-style one.
>
> Spotted by Coverity.
>
> Signed-off-by: Andrew Cooper
> ---
> CC: Jan Beulich
> CC: Wei Liu
> CC: Roger Pau Monné
> CC: Boris Ostrovsky
> CC: Suravee Suthikulpanit
> CC: Brian Woods
> CC: Juergen Gross
> CC: Razvan Cojocaru
> CC: Tamas K Lengyel
>
> This wants backporting to Xen 4.11
> ---
> xen/arch/x86/hvm/svm/svm.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
> index 2584b90..e21091c 100644
> --- a/xen/arch/x86/hvm/svm/svm.c
> +++ b/xen/arch/x86/hvm/svm/svm.c
> @@ -2758,6 +2758,9 @@ void svm_vmexit_handler(struct cpu_user_regs *regs)
> {
> trap_type = X86_EVENTTYPE_PRI_SW_EXCEPTION;
> inst_len = svm_get_insn_len(v, INSTR_ICEBP);
> +
> +if ( !instr_len )
> +break;
> }
>
> rc = hvm_monitor_debug(regs->rip,
>
Acked-by: Brian Woods
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH for-4.12] x86/svm: Fix handling of ICEBP intercepts
>>> On 04.02.19 at 20:44, wrote: > On 04/02/2019 09:16, Jan Beulich wrote: > On 01.02.19 at 18:09, wrote: >>> A subsequent #DB getting raised causes #GP to turn into #DF. >> The table on the #DF page clearly says >> otherwise, at least according to my reading. > > Hmm - so it does. Looks like we've got a 3rd bug, in the general > exception combining logic. I've sent a patch. I'd like to note though that svm_inject_event()'s behavior is problematic in this regard as well: Fiddling with RIP and NEXTRIP without knowing whether a second exception may replace the first is at least a latent issue. I think any such adjustments should only ever be done at the point no further need for exception injection can possibly arise, i.e. presumably in svm_vmenter_helper(). Jan ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH for-4.12] x86/svm: Fix handling of ICEBP intercepts
>>> On 04.02.19 at 20:44, wrote: > On 04/02/2019 09:16, Jan Beulich wrote: > On 01.02.19 at 18:09, wrote: >>> On 01/02/2019 16:55, Jan Beulich wrote: >>> On 01.02.19 at 17:25, wrote: > If it were just getting insn_len incorrectly as 0, then the guest would > livelock as we wouldn't inject the #DB with trap semantics it requires, I'm confused again: Why trap semantics? The ICEBP has fault semantics as you confirmed above. >>> The ICEBP intercept has fault semantics. An ICEBP instruction executing >>> in the guest has trap semantics. >> Oh, okay - I was mis-remembering this aspect. >> > but as the #GP is already raised, this will combine to #DF. How that? #DB is a benign exception, so according to the table on the #DF page in the SDM, with #GP it shouldn't combine to #DF. >>> #GP is raised first. It is contributory. >>> >>> A subsequent #DB getting raised causes #GP to turn into #DF. >> That's based on what? > > Based on actually trying this error scenario. > > (d1) --- Xen Test Framework --- > (d1) Environment: HVM 64bit (Long mode 4 levels) > (d1) Hello World > (XEN) ** Got ICEBP intercept > (d1) ** > (d1) PANIC: Unhandled exception at 0046:0008 > (d1) Vec 8 #DF[4740] > (d1) ** > > Clearly something is off-by-one in the eventual stack frame, which > probably means we've got a a bug in svm_inject_event(). I suspect the > escalation to #DF doesn't overwrite the #DB's "no error code" > information, but I've not investigated yet. I think this is because svm_inject_event(), when calling hvm_combine_hw_exceptions(), legitimately assumes the new event can only possibly be a HW_EXCEPTION. It therefore doesn't overwrite _event.type, which in this (bogus) case is PRI_SW_EXCEPTION. As a result eventinj.fields.ev won't get set, as we won't reach the subsequent switch()'s default case. I don't think that's worth fixing though, as the assumption is correct (afaict). If anything we could add a respective ASSERT(), but of course only once the bad call is gone. Jan ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH for-4.12] x86/svm: Fix handling of ICEBP intercepts
On 04/02/2019 09:16, Jan Beulich wrote: On 01.02.19 at 18:09, wrote: >> On 01/02/2019 16:55, Jan Beulich wrote: >> On 01.02.19 at 17:25, wrote: If it were just getting insn_len incorrectly as 0, then the guest would livelock as we wouldn't inject the #DB with trap semantics it requires, >>> I'm confused again: Why trap semantics? The ICEBP has fault >>> semantics as you confirmed above. >> The ICEBP intercept has fault semantics. An ICEBP instruction executing >> in the guest has trap semantics. > Oh, okay - I was mis-remembering this aspect. > but as the #GP is already raised, this will combine to #DF. >>> How that? #DB is a benign exception, so according to the table on the >>> #DF page in the SDM, with #GP it shouldn't combine to #DF. >> #GP is raised first. It is contributory. >> >> A subsequent #DB getting raised causes #GP to turn into #DF. > That's based on what? Based on actually trying this error scenario. (d1) --- Xen Test Framework --- (d1) Environment: HVM 64bit (Long mode 4 levels) (d1) Hello World (XEN) ** Got ICEBP intercept (d1) ** (d1) PANIC: Unhandled exception at 0046:0008 (d1) Vec 8 #DF[4740] (d1) ** Clearly something is off-by-one in the eventual stack frame, which probably means we've got a a bug in svm_inject_event(). I suspect the escalation to #DF doesn't overwrite the #DB's "no error code" information, but I've not investigated yet. > The table on the #DF page clearly says > otherwise, at least according to my reading. Hmm - so it does. Looks like we've got a 3rd bug, in the general exception combining logic. > But in the end there shouldn't be any attempt to inject #DB anyway > when #GP is already pending, irrespective of the fact that this #GP > is non-architectural. Correct. That bug is what this patch is fixing. ~Andrew ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH for-4.12] x86/svm: Fix handling of ICEBP intercepts
>>> On 01.02.19 at 18:09, wrote: > On 01/02/2019 16:55, Jan Beulich wrote: > On 01.02.19 at 17:25, wrote: >>> If it were just getting insn_len incorrectly as 0, then the guest would >>> livelock as we wouldn't inject the #DB with trap semantics it requires, >> I'm confused again: Why trap semantics? The ICEBP has fault >> semantics as you confirmed above. > > The ICEBP intercept has fault semantics. An ICEBP instruction executing > in the guest has trap semantics. Oh, okay - I was mis-remembering this aspect. >>> but as the #GP is already raised, this will combine to #DF. >> How that? #DB is a benign exception, so according to the table on the >> #DF page in the SDM, with #GP it shouldn't combine to #DF. > > #GP is raised first. It is contributory. > > A subsequent #DB getting raised causes #GP to turn into #DF. That's based on what? The table on the #DF page clearly says otherwise, at least according to my reading. But in the end there shouldn't be any attempt to inject #DB anyway when #GP is already pending, irrespective of the fact that this #GP is non-architectural. Jan ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH for-4.12] x86/svm: Fix handling of ICEBP intercepts
On 01/02/2019 16:55, Jan Beulich wrote: On 01.02.19 at 17:25, wrote: >> On 01/02/2019 15:58, Jan Beulich wrote: >> On 01.02.19 at 15:49, wrote: c/s 9338a37d "x86/svm: implement debug events" added support for introspecting ICEBP debug exceptions, but didn't account for the fact that svm_get_insn_len() (previously __get_instruction_length) can fail and may already raise #GP for the guest. If svm_get_insn_len() fails, return back to guest context rather than continuing and mistaking a trap-style VMExit for a fault-style one. >>> My reading of the last part of this sentence is that the exit in >>> question is a trap-style one. Is my English failing me here? >> Your reading of my sentence is correct, but I was confused when writing it. >> >> ICEBP is a fault-style intercept. >> >> However, when svm_get_insn_len() fails, it will inject #GP and return > s/will/may/ afaict Oh true - this is also problematic for all other cases. I'll need to fix that up as well, > >> 0. This then gets passed into hvm_monitor_debug() or the #DB >> re-injected as-was. >> >> If it were just getting insn_len incorrectly as 0, then the guest would >> livelock as we wouldn't inject the #DB with trap semantics it requires, > I'm confused again: Why trap semantics? The ICEBP has fault > semantics as you confirmed above. The ICEBP intercept has fault semantics. An ICEBP instruction executing in the guest has trap semantics. Xen has to make up the difference as part of re-injecting the #DB (which is extra complicated due to AMD's even injection not handing X86_EVENTTYPE_PRI_SW_EXCEPTION in a sensible way). > >> but as the #GP is already raised, this will combine to #DF. > How that? #DB is a benign exception, so according to the table on the > #DF page in the SDM, with #GP it shouldn't combine to #DF. #GP is raised first. It is contributory. A subsequent #DB getting raised causes #GP to turn into #DF. > Also, if live-locking the guest is a concern, then (as said before) how > can simply re-entering the guest be the right solution? It isn't, given your observation that there is a path out of svm_get_insn_len() which return 0 and don't raise #GP, but that is an orthogonal bug to fix. ~Andrew ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH for-4.12] x86/svm: Fix handling of ICEBP intercepts
>>> On 01.02.19 at 17:25, wrote: > On 01/02/2019 15:58, Jan Beulich wrote: > On 01.02.19 at 15:49, wrote: >>> c/s 9338a37d "x86/svm: implement debug events" added support for >>> introspecting >>> ICEBP debug exceptions, but didn't account for the fact that >>> svm_get_insn_len() (previously __get_instruction_length) can fail and may >>> already raise #GP for the guest. >>> >>> If svm_get_insn_len() fails, return back to guest context rather than >>> continuing and mistaking a trap-style VMExit for a fault-style one. >> My reading of the last part of this sentence is that the exit in >> question is a trap-style one. Is my English failing me here? > > Your reading of my sentence is correct, but I was confused when writing it. > > ICEBP is a fault-style intercept. > > However, when svm_get_insn_len() fails, it will inject #GP and return s/will/may/ afaict > 0. This then gets passed into hvm_monitor_debug() or the #DB > re-injected as-was. > > If it were just getting insn_len incorrectly as 0, then the guest would > livelock as we wouldn't inject the #DB with trap semantics it requires, I'm confused again: Why trap semantics? The ICEBP has fault semantics as you confirmed above. > but as the #GP is already raised, this will combine to #DF. How that? #DB is a benign exception, so according to the table on the #DF page in the SDM, with #GP it shouldn't combine to #DF. Also, if live-locking the guest is a concern, then (as said before) how can simply re-entering the guest be the right solution? Jan ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH for-4.12] x86/svm: Fix handling of ICEBP intercepts
On 01/02/2019 15:58, Jan Beulich wrote: On 01.02.19 at 15:49, wrote: >> c/s 9338a37d "x86/svm: implement debug events" added support for >> introspecting >> ICEBP debug exceptions, but didn't account for the fact that >> svm_get_insn_len() (previously __get_instruction_length) can fail and may >> already raise #GP for the guest. >> >> If svm_get_insn_len() fails, return back to guest context rather than >> continuing and mistaking a trap-style VMExit for a fault-style one. > My reading of the last part of this sentence is that the exit in > question is a trap-style one. Is my English failing me here? Your reading of my sentence is correct, but I was confused when writing it. ICEBP is a fault-style intercept. However, when svm_get_insn_len() fails, it will inject #GP and return 0. This then gets passed into hvm_monitor_debug() or the #DB re-injected as-was. If it were just getting insn_len incorrectly as 0, then the guest would livelock as we wouldn't inject the #DB with trap semantics it requires, but as the #GP is already raised, this will combine to #DF. ~Andrew ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH for-4.12] x86/svm: Fix handling of ICEBP intercepts
>>> On 01.02.19 at 15:49, wrote: > c/s 9338a37d "x86/svm: implement debug events" added support for introspecting > ICEBP debug exceptions, but didn't account for the fact that > svm_get_insn_len() (previously __get_instruction_length) can fail and may > already raise #GP for the guest. > > If svm_get_insn_len() fails, return back to guest context rather than > continuing and mistaking a trap-style VMExit for a fault-style one. My reading of the last part of this sentence is that the exit in question is a trap-style one. Is my English failing me here? Because if so, why would there be any call to svm_get_insn_len() in the first place? For a trap-style exit guest RIP should already point past the insn, and hence the debug mode checking ought to never succeed (unless there's a second ICEBP following the first one immediately). If, otoh, it really is a fault-style exit (which was my understanding so far), how is exiting back to guest context going to do any good, if svm_get_insn_len() did _not_ raise #GP(0)? We'd just see the same exit right away. Jan ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH for-4.12] x86/svm: Fix handling of ICEBP intercepts
On Fri, Feb 1, 2019 at 7:49 AM Andrew Cooper wrote:
>
> c/s 9338a37d "x86/svm: implement debug events" added support for introspecting
> ICEBP debug exceptions, but didn't account for the fact that
> svm_get_insn_len() (previously __get_instruction_length) can fail and may
> already raise #GP for the guest.
>
> If svm_get_insn_len() fails, return back to guest context rather than
> continuing and mistaking a trap-style VMExit for a fault-style one.
>
> Spotted by Coverity.
>
> Signed-off-by: Andrew Cooper
> ---
> CC: Jan Beulich
> CC: Wei Liu
> CC: Roger Pau Monné
> CC: Boris Ostrovsky
> CC: Suravee Suthikulpanit
> CC: Brian Woods
> CC: Juergen Gross
> CC: Razvan Cojocaru
> CC: Tamas K Lengyel
>
> This wants backporting to Xen 4.11
> ---
> xen/arch/x86/hvm/svm/svm.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
> index 2584b90..e21091c 100644
> --- a/xen/arch/x86/hvm/svm/svm.c
> +++ b/xen/arch/x86/hvm/svm/svm.c
> @@ -2758,6 +2758,9 @@ void svm_vmexit_handler(struct cpu_user_regs *regs)
> {
> trap_type = X86_EVENTTYPE_PRI_SW_EXCEPTION;
> inst_len = svm_get_insn_len(v, INSTR_ICEBP);
> +
> +if ( !instr_len )
Should that have been inst_len instead of instr_len?
Tamas
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH for-4.12] x86/svm: Fix handling of ICEBP intercepts
On 2/1/19 4:49 PM, Andrew Cooper wrote:
c/s 9338a37d "x86/svm: implement debug events" added support for introspecting
ICEBP debug exceptions, but didn't account for the fact that
svm_get_insn_len() (previously __get_instruction_length) can fail and may
already raise #GP for the guest.
If svm_get_insn_len() fails, return back to guest context rather than
continuing and mistaking a trap-style VMExit for a fault-style one.
Spotted by Coverity.
Signed-off-by: Andrew Cooper
---
CC: Jan Beulich
CC: Wei Liu
CC: Roger Pau Monné
CC: Boris Ostrovsky
CC: Suravee Suthikulpanit
CC: Brian Woods
CC: Juergen Gross
CC: Razvan Cojocaru
CC: Tamas K Lengyel
This wants backporting to Xen 4.11
---
xen/arch/x86/hvm/svm/svm.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
index 2584b90..e21091c 100644
--- a/xen/arch/x86/hvm/svm/svm.c
+++ b/xen/arch/x86/hvm/svm/svm.c
@@ -2758,6 +2758,9 @@ void svm_vmexit_handler(struct cpu_user_regs *regs)
{
trap_type = X86_EVENTTYPE_PRI_SW_EXCEPTION;
inst_len = svm_get_insn_len(v, INSTR_ICEBP);
+
+if ( !instr_len )
+break;
}
rc = hvm_monitor_debug(regs->rip,
Reviewed-by: Razvan Cojocaru
Thanks,
Razvan
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH for-4.12] x86/svm: Fix handling of ICEBP intercepts
On 01/02/2019 15:49, Andrew Cooper wrote: > c/s 9338a37d "x86/svm: implement debug events" added support for introspecting > ICEBP debug exceptions, but didn't account for the fact that > svm_get_insn_len() (previously __get_instruction_length) can fail and may > already raise #GP for the guest. > > If svm_get_insn_len() fails, return back to guest context rather than > continuing and mistaking a trap-style VMExit for a fault-style one. > > Spotted by Coverity. > > Signed-off-by: Andrew Cooper Release-acked-by: Juergen Gross Juergen ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
[Xen-devel] [PATCH for-4.12] x86/svm: Fix handling of ICEBP intercepts
c/s 9338a37d "x86/svm: implement debug events" added support for introspecting
ICEBP debug exceptions, but didn't account for the fact that
svm_get_insn_len() (previously __get_instruction_length) can fail and may
already raise #GP for the guest.
If svm_get_insn_len() fails, return back to guest context rather than
continuing and mistaking a trap-style VMExit for a fault-style one.
Spotted by Coverity.
Signed-off-by: Andrew Cooper
---
CC: Jan Beulich
CC: Wei Liu
CC: Roger Pau Monné
CC: Boris Ostrovsky
CC: Suravee Suthikulpanit
CC: Brian Woods
CC: Juergen Gross
CC: Razvan Cojocaru
CC: Tamas K Lengyel
This wants backporting to Xen 4.11
---
xen/arch/x86/hvm/svm/svm.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
index 2584b90..e21091c 100644
--- a/xen/arch/x86/hvm/svm/svm.c
+++ b/xen/arch/x86/hvm/svm/svm.c
@@ -2758,6 +2758,9 @@ void svm_vmexit_handler(struct cpu_user_regs *regs)
{
trap_type = X86_EVENTTYPE_PRI_SW_EXCEPTION;
inst_len = svm_get_insn_len(v, INSTR_ICEBP);
+
+if ( !instr_len )
+break;
}
rc = hvm_monitor_debug(regs->rip,
--
2.1.4
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH for-4.12] x86/svm: Fix handling of ICEBP intercepts
On 01/02/2019 14:52, Tamas K Lengyel wrote:
> On Fri, Feb 1, 2019 at 7:49 AM Andrew Cooper
> wrote:
>> c/s 9338a37d "x86/svm: implement debug events" added support for
>> introspecting
>> ICEBP debug exceptions, but didn't account for the fact that
>> svm_get_insn_len() (previously __get_instruction_length) can fail and may
>> already raise #GP for the guest.
>>
>> If svm_get_insn_len() fails, return back to guest context rather than
>> continuing and mistaking a trap-style VMExit for a fault-style one.
>>
>> Spotted by Coverity.
>>
>> Signed-off-by: Andrew Cooper
>> ---
>> CC: Jan Beulich
>> CC: Wei Liu
>> CC: Roger Pau Monné
>> CC: Boris Ostrovsky
>> CC: Suravee Suthikulpanit
>> CC: Brian Woods
>> CC: Juergen Gross
>> CC: Razvan Cojocaru
>> CC: Tamas K Lengyel
>>
>> This wants backporting to Xen 4.11
>> ---
>> xen/arch/x86/hvm/svm/svm.c | 3 +++
>> 1 file changed, 3 insertions(+)
>>
>> diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
>> index 2584b90..e21091c 100644
>> --- a/xen/arch/x86/hvm/svm/svm.c
>> +++ b/xen/arch/x86/hvm/svm/svm.c
>> @@ -2758,6 +2758,9 @@ void svm_vmexit_handler(struct cpu_user_regs *regs)
>> {
>> trap_type = X86_EVENTTYPE_PRI_SW_EXCEPTION;
>> inst_len = svm_get_insn_len(v, INSTR_ICEBP);
>> +
>> +if ( !instr_len )
> Should that have been inst_len instead of instr_len?
Bah - serves me right not to refresh my patch before sending it. Yes -
this is a typo.
~Andrew
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
