| Message ID | 20240930161837.1248144-2-andrew.cooper3@citrix.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | x86/pv: Fixes to guest_io_okay() | expand |
On 30.09.2024 18:18, Andrew Cooper wrote: > In order to fix a bug with guest_io_okay() (subsequent patch), rework > guest_io_okay() to take in an emulation context, and return X86EMUL_* rather > than a boolean. > > For the failing case, take the opporunity to inject #GP explicitly, rather > than returning X86EMUL_UNHANDLEABLE. There is a logical difference between > "we know what this is, and it's #GP", vs "we don't know what this is". > > There is no change in practice as emulation is the final step on general #GP > resolution, but returning X86EMUL_UNHANDLEABLE would be a latent bug if a > subsequent action were to appear. > > No practical change. I think there is a small functional difference, see below. > --- a/xen/arch/x86/pv/emul-priv-op.c > +++ b/xen/arch/x86/pv/emul-priv-op.c > @@ -156,14 +156,16 @@ static bool iopl_ok(const struct vcpu *v, const struct cpu_user_regs *regs) > } > > /* Has the guest requested sufficient permission for this I/O access? */ > -static bool guest_io_okay(unsigned int port, unsigned int bytes, > - struct vcpu *v, struct cpu_user_regs *regs) > +static int guest_io_okay(unsigned int port, unsigned int bytes, > + struct x86_emulate_ctxt *ctxt) > { > + struct cpu_user_regs *regs = ctxt->regs; const? > @@ -190,10 +192,12 @@ static bool guest_io_okay(unsigned int port, unsigned int bytes, > toggle_guest_pt(v); > > if ( (x.mask & (((1 << bytes) - 1) << (port & 7))) == 0 ) > - return true; > + return X86EMUL_OKAY; > } > > - return false; > + x86_emul_hw_exception(X86_EXC_GP, 0, ctxt); do_general_protection() has /* Pass on GPF as is. */ pv_inject_hw_exception(X86_EXC_GP, regs->error_code); which may make a difference in case the insn changes under our feet. The new behavior may even be deemed better, but the description then would want to admit to this slight functional change. Then Reviewed-by: Jan Beulich <jbeulich@suse.com> Jan
On 01/10/2024 8:11 am, Jan Beulich wrote: > On 30.09.2024 18:18, Andrew Cooper wrote: >> @@ -190,10 +192,12 @@ static bool guest_io_okay(unsigned int port, unsigned int bytes, >> toggle_guest_pt(v); >> >> if ( (x.mask & (((1 << bytes) - 1) << (port & 7))) == 0 ) >> - return true; >> + return X86EMUL_OKAY; >> } >> >> - return false; >> + x86_emul_hw_exception(X86_EXC_GP, 0, ctxt); > do_general_protection() has > > /* Pass on GPF as is. */ > pv_inject_hw_exception(X86_EXC_GP, regs->error_code); > > which may make a difference in case the insn changes under our feet. It would make a difference if we chose to raise #GP[non-0]. But, see how the call to pv_emulate_privileged_op() is guarded on error_code == 0. Prior X86EMUL_UNHANDLEABLE can't ever have raised anything other than #GP[0], (excusing cases of memory corruption in regs->error_code). So, there is not a change in behaviour, even if the reason why is less-than-obvious. ~Andrew
On 01.10.2024 12:02, Andrew Cooper wrote: > On 01/10/2024 8:11 am, Jan Beulich wrote: >> On 30.09.2024 18:18, Andrew Cooper wrote: >>> @@ -190,10 +192,12 @@ static bool guest_io_okay(unsigned int port, unsigned int bytes, >>> toggle_guest_pt(v); >>> >>> if ( (x.mask & (((1 << bytes) - 1) << (port & 7))) == 0 ) >>> - return true; >>> + return X86EMUL_OKAY; >>> } >>> >>> - return false; >>> + x86_emul_hw_exception(X86_EXC_GP, 0, ctxt); >> do_general_protection() has >> >> /* Pass on GPF as is. */ >> pv_inject_hw_exception(X86_EXC_GP, regs->error_code); >> >> which may make a difference in case the insn changes under our feet. > > It would make a difference if we chose to raise #GP[non-0]. > > But, see how the call to pv_emulate_privileged_op() is guarded on > error_code == 0. Oh, good point - I overlooked that. > Prior X86EMUL_UNHANDLEABLE can't ever have raised anything other than > #GP[0], (excusing cases of memory corruption in regs->error_code). > > So, there is not a change in behaviour, even if the reason why is > less-than-obvious. I agree then. Jan
diff --git a/xen/arch/x86/pv/emul-priv-op.c b/xen/arch/x86/pv/emul-priv-op.c index b90f745c75ea..978bd6c0775f 100644 --- a/xen/arch/x86/pv/emul-priv-op.c +++ b/xen/arch/x86/pv/emul-priv-op.c @@ -156,14 +156,16 @@ static bool iopl_ok(const struct vcpu *v, const struct cpu_user_regs *regs) } /* Has the guest requested sufficient permission for this I/O access? */ -static bool guest_io_okay(unsigned int port, unsigned int bytes, - struct vcpu *v, struct cpu_user_regs *regs) +static int guest_io_okay(unsigned int port, unsigned int bytes, + struct x86_emulate_ctxt *ctxt) { + struct cpu_user_regs *regs = ctxt->regs; + struct vcpu *v = current; /* If in user mode, switch to kernel mode just to read I/O bitmap. */ const bool user_mode = !(v->arch.flags & TF_kernel_mode); if ( iopl_ok(v, regs) ) - return true; + return X86EMUL_OKAY; if ( (port + bytes) <= v->arch.pv.iobmp_limit ) { @@ -190,10 +192,12 @@ static bool guest_io_okay(unsigned int port, unsigned int bytes, toggle_guest_pt(v); if ( (x.mask & (((1 << bytes) - 1) << (port & 7))) == 0 ) - return true; + return X86EMUL_OKAY; } - return false; + x86_emul_hw_exception(X86_EXC_GP, 0, ctxt); + + return X86EMUL_EXCEPTION; } /* Has the administrator granted sufficient permission for this I/O access? */ @@ -353,12 +357,14 @@ static int cf_check read_io( struct priv_op_ctxt *poc = container_of(ctxt, struct priv_op_ctxt, ctxt); struct vcpu *curr = current; struct domain *currd = current->domain; + int rc; /* INS must not come here. */ ASSERT((ctxt->opcode & ~9) == 0xe4); - if ( !guest_io_okay(port, bytes, curr, ctxt->regs) ) - return X86EMUL_UNHANDLEABLE; + rc = guest_io_okay(port, bytes, ctxt); + if ( rc != X86EMUL_OKAY ) + return rc; poc->bpmatch = check_guest_io_breakpoint(curr, port, bytes); @@ -458,12 +464,14 @@ static int cf_check write_io( struct priv_op_ctxt *poc = container_of(ctxt, struct priv_op_ctxt, ctxt); struct vcpu *curr = current; struct domain *currd = current->domain; + int rc; /* OUTS must not come here. */ ASSERT((ctxt->opcode & ~9) == 0xe6); - if ( !guest_io_okay(port, bytes, curr, ctxt->regs) ) - return X86EMUL_UNHANDLEABLE; + rc = guest_io_okay(port, bytes, ctxt); + if ( rc != X86EMUL_OKAY ) + return rc; poc->bpmatch = check_guest_io_breakpoint(curr, port, bytes); @@ -612,8 +620,9 @@ static int cf_check rep_ins( *reps = 0; - if ( !guest_io_okay(port, bytes_per_rep, curr, ctxt->regs) ) - return X86EMUL_UNHANDLEABLE; + rc = guest_io_okay(port, bytes_per_rep, ctxt); + if ( rc != X86EMUL_OKAY ) + return rc; rc = read_segment(x86_seg_es, &sreg, ctxt); if ( rc != X86EMUL_OKAY ) @@ -678,8 +687,9 @@ static int cf_check rep_outs( *reps = 0; - if ( !guest_io_okay(port, bytes_per_rep, curr, ctxt->regs) ) - return X86EMUL_UNHANDLEABLE; + rc = guest_io_okay(port, bytes_per_rep, ctxt); + if ( rc != X86EMUL_OKAY ) + return rc; rc = read_segment(seg, &sreg, ctxt); if ( rc != X86EMUL_OKAY )
In order to fix a bug with guest_io_okay() (subsequent patch), rework guest_io_okay() to take in an emulation context, and return X86EMUL_* rather than a boolean. For the failing case, take the opporunity to inject #GP explicitly, rather than returning X86EMUL_UNHANDLEABLE. There is a logical difference between "we know what this is, and it's #GP", vs "we don't know what this is". There is no change in practice as emulation is the final step on general #GP resolution, but returning X86EMUL_UNHANDLEABLE would be a latent bug if a subsequent action were to appear. No practical change. Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> --- CC: Jan Beulich <JBeulich@suse.com> CC: Roger Pau Monné <roger.pau@citrix.com> --- xen/arch/x86/pv/emul-priv-op.c | 36 ++++++++++++++++++++++------------ 1 file changed, 23 insertions(+), 13 deletions(-)