| Message ID | 81428267-e963-4403-989d-d96fb0b59ffc@suse.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | x86emul: correct put_fpu()'s segment selector handling | expand |
On 07/01/2025 2:33 pm, Jan Beulich wrote: > All selector fields under ctxt->regs are (normally) poisoned in the HVM > case, and the four ones besides CS and SS are potentially stale for PV. > Avoid using them in the hypervisor incarnation of the emulator, when > trying to cover for a missing ->read_segment() hook. > > To make sure there's always a valid ->read_segment() handler for all HVM > cases, add a respective function to shadow code, even if it is not > expected for FPU insns to be used to update page tables. > > Fixes: 0711b59b858a ("x86emul: correct FPU code/data pointers and opcode handling") > Reported-by: Andrew Cooper <andrew.cooper3@citrix.com> > Signed-off-by: Jan Beulich <jbeulich@suse.com> > --- > The code comment may want adjusting in the course of FRED work. It compiles when displacing my temporary patch in the FRED branch. I've not got the ABI compatibility in userspace working yet, but regs->{ds,es,fs,gs} will be staying, so the #else case should be fine (assuming they're populated properly). So, tentatively, Acked-by: Andrew Cooper <andrew.cooper3@citrix.com> That said, I think it would be nicer to see about excluding the FPU in these cases. Both cases lacking read_segment() hooks are pagetable emulation, and I'd say it's more likely to be code corruption than there actually being x87 instructions in the middle of a dual 32bit PAE update. ~Andrew
On 07.01.2025 16:37, Andrew Cooper wrote: > On 07/01/2025 2:33 pm, Jan Beulich wrote: >> All selector fields under ctxt->regs are (normally) poisoned in the HVM >> case, and the four ones besides CS and SS are potentially stale for PV. >> Avoid using them in the hypervisor incarnation of the emulator, when >> trying to cover for a missing ->read_segment() hook. >> >> To make sure there's always a valid ->read_segment() handler for all HVM >> cases, add a respective function to shadow code, even if it is not >> expected for FPU insns to be used to update page tables. >> >> Fixes: 0711b59b858a ("x86emul: correct FPU code/data pointers and opcode handling") >> Reported-by: Andrew Cooper <andrew.cooper3@citrix.com> >> Signed-off-by: Jan Beulich <jbeulich@suse.com> >> --- >> The code comment may want adjusting in the course of FRED work. > > It compiles when displacing my temporary patch in the FRED branch. I've > not got the ABI compatibility in userspace working yet, but > regs->{ds,es,fs,gs} will be staying, so the #else case should be fine > (assuming they're populated properly). > > So, tentatively, Acked-by: Andrew Cooper <andrew.cooper3@citrix.com> Thanks. > That said, I think it would be nicer to see about excluding the FPU in > these cases. Both cases lacking read_segment() hooks are pagetable > emulation, and I'd say it's more likely to be code corruption than there > actually being x87 instructions in the middle of a dual 32bit PAE update. I considered this case, but decided against going this route. We shouldn't be stricter than necessary towards what we permit guests to do, however odd it might look to us. Jan
On 07/01/2025 3:41 pm, Jan Beulich wrote: > On 07.01.2025 16:37, Andrew Cooper wrote: >> On 07/01/2025 2:33 pm, Jan Beulich wrote: >>> All selector fields under ctxt->regs are (normally) poisoned in the HVM >>> case, and the four ones besides CS and SS are potentially stale for PV. >>> Avoid using them in the hypervisor incarnation of the emulator, when >>> trying to cover for a missing ->read_segment() hook. >>> >>> To make sure there's always a valid ->read_segment() handler for all HVM >>> cases, add a respective function to shadow code, even if it is not >>> expected for FPU insns to be used to update page tables. >>> >>> Fixes: 0711b59b858a ("x86emul: correct FPU code/data pointers and opcode handling") >>> Reported-by: Andrew Cooper <andrew.cooper3@citrix.com> >>> Signed-off-by: Jan Beulich <jbeulich@suse.com> >>> --- >>> The code comment may want adjusting in the course of FRED work. >> It compiles when displacing my temporary patch in the FRED branch. I've >> not got the ABI compatibility in userspace working yet, but >> regs->{ds,es,fs,gs} will be staying, so the #else case should be fine >> (assuming they're populated properly). >> >> So, tentatively, Acked-by: Andrew Cooper <andrew.cooper3@citrix.com> > Thanks. > >> That said, I think it would be nicer to see about excluding the FPU in >> these cases. Both cases lacking read_segment() hooks are pagetable >> emulation, and I'd say it's more likely to be code corruption than there >> actually being x87 instructions in the middle of a dual 32bit PAE update. > I considered this case, but decided against going this route. We shouldn't > be stricter than necessary towards what we permit guests to do, however odd > it might look to us. I suppose so, but then I wonder if we ought to be setting up more infrastructure by default for emulations. We've got an awful lot of the emulator which has fallback paths upon fallback paths, and probably is not as well tested as it ought to be. ~Andrew
--- a/xen/arch/x86/mm/shadow/hvm.c +++ b/xen/arch/x86/mm/shadow/hvm.c @@ -287,11 +287,29 @@ hvm_emulate_cmpxchg(enum x86_segment seg return rc; } +static int cf_check +hvm_emulate_read_segment(enum x86_segment seg, + struct segment_register *reg, + struct x86_emulate_ctxt *ctxt) +{ + struct sh_emulate_ctxt *sh_ctxt = + container_of(ctxt, struct sh_emulate_ctxt, ctxt); + const struct segment_register *sreg = hvm_get_seg_reg(seg, sh_ctxt); + + if ( IS_ERR(sreg) ) + return -PTR_ERR(sreg); + + *reg = *sreg; + + return X86EMUL_OKAY; +} + static const struct x86_emulate_ops hvm_shadow_emulator_ops = { .read = hvm_emulate_read, .insn_fetch = hvm_emulate_insn_fetch, .write = hvm_emulate_write, .cmpxchg = hvm_emulate_cmpxchg, + .read_segment = hvm_emulate_read_segment, }; const struct x86_emulate_ops *shadow_init_emulation( --- a/xen/arch/x86/x86_emulate/x86_emulate.c +++ b/xen/arch/x86/x86_emulate/x86_emulate.c @@ -447,14 +447,37 @@ static void put_fpu( if ( state->ea.type == OP_MEM ) { aux.dp = state->ea.mem.off; - if ( ops->read_segment && - ops->read_segment(state->ea.mem.seg, &sreg, - ctxt) == X86EMUL_OKAY ) + if ( state->ea.mem.seg == x86_seg_cs ) + aux.ds = aux.cs; + else if ( ops->read_segment && + ops->read_segment(state->ea.mem.seg, &sreg, + ctxt) == X86EMUL_OKAY ) aux.ds = sreg.sel; +#ifdef __XEN__ + /* + * While generally the expectation is that input structures are + * fully populated, the selector fields under ctxt->regs normally + * aren't set, with the exception of CS and SS for PV domains. + * Read the real selector registers for PV, and assert that HVM + * invocations always set a properly functioning ->read_segment() + * hook. + */ + else if ( is_pv_vcpu(current) ) + switch ( state->ea.mem.seg ) + { + case x86_seg_ds: aux.ds = read_sreg(ds); break; + case x86_seg_es: aux.ds = read_sreg(es); break; + case x86_seg_fs: aux.ds = read_sreg(fs); break; + case x86_seg_gs: aux.ds = read_sreg(gs); break; + case x86_seg_ss: aux.ds = ctxt->regs->ss; break; + default: ASSERT_UNREACHABLE(); break; + } + else + ASSERT_UNREACHABLE(); +#else else switch ( state->ea.mem.seg ) { - case x86_seg_cs: aux.ds = ctxt->regs->cs; break; case x86_seg_ds: aux.ds = ctxt->regs->ds; break; case x86_seg_es: aux.ds = ctxt->regs->es; break; case x86_seg_fs: aux.ds = ctxt->regs->fs; break; @@ -462,6 +485,7 @@ static void put_fpu( case x86_seg_ss: aux.ds = ctxt->regs->ss; break; default: ASSERT_UNREACHABLE(); break; } +#endif aux.dval = true; } ops->put_fpu(ctxt, X86EMUL_FPU_none, &aux);
All selector fields under ctxt->regs are (normally) poisoned in the HVM case, and the four ones besides CS and SS are potentially stale for PV. Avoid using them in the hypervisor incarnation of the emulator, when trying to cover for a missing ->read_segment() hook. To make sure there's always a valid ->read_segment() handler for all HVM cases, add a respective function to shadow code, even if it is not expected for FPU insns to be used to update page tables. Fixes: 0711b59b858a ("x86emul: correct FPU code/data pointers and opcode handling") Reported-by: Andrew Cooper <andrew.cooper3@citrix.com> Signed-off-by: Jan Beulich <jbeulich@suse.com> --- The code comment may want adjusting in the course of FRED work.