| Message ID | e79b3b8e-666e-b94f-d2c8-1ec2f6f5453f@suse.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | x86emul: avoid triggering event related assertions | expand |
On 06/04/2023 3:15 pm, Jan Beulich wrote: > The assertion at the end of x86_emulate_wrapper() as well as the ones > in x86_emul_{hw_exception,pagefault}() can trigger if we ignore > X86EMUL_EXCEPTION coming back from certain hook functions. Looking at the comment I wrote back then, I don't think I'd considered this case. What I was fixing at the time was the case where hvm_inject_hw_exception() had raised an exception behind the back of the emulator, and any subsequent exception escalates to #DF. I guess the comment wants updating to discuss this problem too, where the hook queued an exception but we didn't squash it properly when deciding to ignore it. As it's debugging-only anyway, it might be worth rearranging the expression to be if ( ctxt->event_pending ) ASSERT(rc == X86EMUL_EXCEPTION); else ASSERT(rc != X86EMUL_EXCEPTION); because it distinguishes the two cases without an intermediate round of debugging. > Squash > exceptions when merely probing MSRs, plus on SWAPGS'es "best effort" > error handling path. > > In adjust_bnd() add another assertion after the read_xcr(0, ...) > invocation, paralleling the one in x86emul_get_fpu() - XCR0 reads should > never fault when XSAVE is (implicitly) known to be available. > > Fixes: 14a6be89ec04 ("x86emul: correct EFLAGS.TF handling") > Fixes: cb2626c75813 ("x86emul: conditionally clear BNDn for branches") > Fixes: 6eb43fcf8a0b ("x86emul: support SWAPGS") > Reported-by: AFL > Signed-off-by: Jan Beulich <jbeulich@suse.com> > --- > EFER reads won't fault in any of the handlers we have, so in principle > the respective check could be omitted (and hence has no Fixes: tag). > Thoughts? We already require LMA as input state, and don't have an execution model where EFER is actually absent in the first place, so passing the whole thing wouldn't really be an issue. I have previously considered doing the same for CR0 too, but at best it's marginal so I haven't got around to trying it. > --- a/xen/arch/x86/x86_emulate/0fae.c > +++ b/xen/arch/x86/x86_emulate/0fae.c > @@ -67,7 +67,10 @@ int x86emul_0fae(struct x86_emulate_stat > cr4 = X86_CR4_OSFXSR; > if ( !ops->read_msr || > ops->read_msr(MSR_EFER, &msr_val, ctxt) != X86EMUL_OKAY ) > + { > + x86_emul_reset_event(ctxt); This is the only path you've introduced that doesn't restrict the reset to the X86EMUL_EXCEPTION case. In principle you can get things like RETRY for introspection. Internally, UNHANDLEABLE is used but I hope that never escapes from guest_{rd,wr}msr(). ~Andrew
On 06.04.2023 17:33, Andrew Cooper wrote: > On 06/04/2023 3:15 pm, Jan Beulich wrote: >> The assertion at the end of x86_emulate_wrapper() as well as the ones >> in x86_emul_{hw_exception,pagefault}() can trigger if we ignore >> X86EMUL_EXCEPTION coming back from certain hook functions. > > Looking at the comment I wrote back then, I don't think I'd considered > this case. > > What I was fixing at the time was the case where > hvm_inject_hw_exception() had raised an exception behind the back of the > emulator, and any subsequent exception escalates to #DF. > > I guess the comment wants updating to discuss this problem too, where > the hook queued an exception but we didn't squash it properly when > deciding to ignore it. Can do. > As it's debugging-only anyway, it might be worth rearranging the > expression to be > > if ( ctxt->event_pending ) > ASSERT(rc == X86EMUL_EXCEPTION); > else > ASSERT(rc != X86EMUL_EXCEPTION); > > because it distinguishes the two cases without an intermediate round of > debugging. Maybe, but I don't think this adjustment would belong here. >> --- >> EFER reads won't fault in any of the handlers we have, so in principle >> the respective check could be omitted (and hence has no Fixes: tag). >> Thoughts? > > We already require LMA as input state, and don't have an execution model > where EFER is actually absent in the first place, so passing the whole > thing wouldn't really be an issue. > > I have previously considered doing the same for CR0 too, but at best > it's marginal so I haven't got around to trying it. Well, that's more longer term plans (and not very clear as you say). I'm afraid this doesn't answer my question, though: Do you think the respective adjustment should stay, or be dropped? >> --- a/xen/arch/x86/x86_emulate/0fae.c >> +++ b/xen/arch/x86/x86_emulate/0fae.c >> @@ -67,7 +67,10 @@ int x86emul_0fae(struct x86_emulate_stat >> cr4 = X86_CR4_OSFXSR; >> if ( !ops->read_msr || >> ops->read_msr(MSR_EFER, &msr_val, ctxt) != X86EMUL_OKAY ) >> + { >> + x86_emul_reset_event(ctxt); > > This is the only path you've introduced that doesn't restrict the reset > to the X86EMUL_EXCEPTION case. Right, other similar ones had to go into individual other patches I have pending. The distinction I made was whether the call was in a helper function (where I want to be careful, because I don't know what state we may have accumulated) vs mainline code. IOW ... > In principle you can get things like RETRY for introspection. > Internally, UNHANDLEABLE is used but I hope that never escapes from > guest_{rd,wr}msr(). ... other errors are possible, yes, but those shouldn't cause an event to be recorded. Hence it seemed reasonable to me to flush events unconditionally here, i.e. even if none is pending in the first place. Jan
--- a/xen/arch/x86/x86_emulate/0f01.c +++ b/xen/arch/x86/x86_emulate/0f01.c @@ -198,8 +198,10 @@ int x86emul_0f01(struct x86_emulate_stat if ( (rc = ops->write_segment(x86_seg_gs, &sreg, ctxt)) != X86EMUL_OKAY ) { - /* Best effort unwind (i.e. no error checking). */ - ops->write_msr(MSR_SHADOW_GS_BASE, msr_val, ctxt); + /* Best effort unwind (i.e. no real error checking). */ + if ( ops->write_msr(MSR_SHADOW_GS_BASE, msr_val, + ctxt) == X86EMUL_EXCEPTION ) + x86_emul_reset_event(ctxt); goto done; } break; --- a/xen/arch/x86/x86_emulate/0fae.c +++ b/xen/arch/x86/x86_emulate/0fae.c @@ -67,7 +67,10 @@ int x86emul_0fae(struct x86_emulate_stat cr4 = X86_CR4_OSFXSR; if ( !ops->read_msr || ops->read_msr(MSR_EFER, &msr_val, ctxt) != X86EMUL_OKAY ) + { + x86_emul_reset_event(ctxt); msr_val = 0; + } if ( !(cr4 & X86_CR4_OSFXSR) || (mode_64bit() && mode_ring0() && (msr_val & EFER_FFXSE)) ) s->op_bytes = offsetof(struct x86_fxsr, xmm[0]); --- a/xen/arch/x86/x86_emulate/x86_emulate.c +++ b/xen/arch/x86/x86_emulate/x86_emulate.c @@ -1177,10 +1177,18 @@ static bool is_branch_step(struct x86_em const struct x86_emulate_ops *ops) { uint64_t debugctl; + int rc = X86EMUL_UNHANDLEABLE; - return ops->read_msr && - ops->read_msr(MSR_IA32_DEBUGCTLMSR, &debugctl, ctxt) == X86EMUL_OKAY && - (debugctl & IA32_DEBUGCTLMSR_BTF); + if ( !ops->read_msr || + (rc = ops->read_msr(MSR_IA32_DEBUGCTLMSR, &debugctl, + ctxt)) != X86EMUL_OKAY ) + { + if ( rc == X86EMUL_EXCEPTION ) + x86_emul_reset_event(ctxt); + debugctl = 0; + } + + return debugctl & IA32_DEBUGCTLMSR_BTF; } static void adjust_bnd(struct x86_emulate_ctxt *ctxt, @@ -1194,13 +1202,21 @@ static void adjust_bnd(struct x86_emulat if ( !ops->read_xcr || ops->read_xcr(0, &xcr0, ctxt) != X86EMUL_OKAY || !(xcr0 & X86_XCR0_BNDREGS) || !(xcr0 & X86_XCR0_BNDCSR) ) + { + ASSERT(!ctxt->event_pending); return; + } if ( !mode_ring0() ) bndcfg = read_bndcfgu(); else if ( !ops->read_msr || - ops->read_msr(MSR_IA32_BNDCFGS, &bndcfg, ctxt) != X86EMUL_OKAY ) + (rc = ops->read_msr(MSR_IA32_BNDCFGS, &bndcfg, + ctxt)) != X86EMUL_OKAY ) + { + if ( rc == X86EMUL_EXCEPTION ) + x86_emul_reset_event(ctxt); return; + } if ( (bndcfg & IA32_BNDCFGS_ENABLE) && !(bndcfg & IA32_BNDCFGS_PRESERVE) ) { /*
The assertion at the end of x86_emulate_wrapper() as well as the ones in x86_emul_{hw_exception,pagefault}() can trigger if we ignore X86EMUL_EXCEPTION coming back from certain hook functions. Squash exceptions when merely probing MSRs, plus on SWAPGS'es "best effort" error handling path. In adjust_bnd() add another assertion after the read_xcr(0, ...) invocation, paralleling the one in x86emul_get_fpu() - XCR0 reads should never fault when XSAVE is (implicitly) known to be available. Fixes: 14a6be89ec04 ("x86emul: correct EFLAGS.TF handling") Fixes: cb2626c75813 ("x86emul: conditionally clear BNDn for branches") Fixes: 6eb43fcf8a0b ("x86emul: support SWAPGS") Reported-by: AFL Signed-off-by: Jan Beulich <jbeulich@suse.com> --- EFER reads won't fault in any of the handlers we have, so in principle the respective check could be omitted (and hence has no Fixes: tag). Thoughts? The Fixes: tags are for the commits introducing the affected code; I'm not entirely sure whether the raising of exceptions from hook functions actually pre-dates them, but even if not the issues were at least latent ones already before.