x86/VMX: sanitize VM86 TSS handling The present way of setting this up is flawed: Leaving the I/O bitmap pointer at zero means that the interrupt redirection bitmap lives outside (ahead of) the allocated space of the TSS. Similarly setting a TSS limit of 255 when only 128 bytes get allocated means that 128 extra bytes may be accessed by the CPU during I/O port access processing. Introduce a new HVM param to set the allocated size of the TSS, and have the hypervisor actually take care of setting namely the I/O bitmap pointer. Both this and the segment limit now take the allocated size into account. Signed-off-by: Jan Beulich --- TBD: Do we really want to re-init the TSS every time we are about to use it? This can happen quite often during boot, especially while grub is running. --- a/tools/firmware/hvmloader/hvmloader.c +++ b/tools/firmware/hvmloader/hvmloader.c @@ -177,18 +177,30 @@ static void cmos_write_memory_size(void) } /* - * Set up an empty TSS area for virtual 8086 mode to use. - * The only important thing is that it musn't have any bits set - * in the interrupt redirection bitmap, so all zeros will do. + * Set up an empty TSS area for virtual 8086 mode to use. Its content is + * going to be managed by Xen, but zero fill it just in case. */ static void init_vm86_tss(void) { +/* + * Have the TSS cover the ISA port range, which makes it + * - 104 bytes base structure + * - 32 bytes interrupt redirection bitmap + * - 128 bytes I/O bitmap + * - one trailing byte + * or a total of to 265 bytes. As it needs to be be a power of two for + * now (or else the alignment parameter to mem_alloc() needs adjustment), + * this ends up requiring 512 bytes. + */ +#define TSS_SIZE 512 void *tss; - tss = mem_alloc(128, 128); - memset(tss, 0, 128); + tss = mem_alloc(TSS_SIZE, TSS_SIZE); + memset(tss, 0, TSS_SIZE); hvm_param_set(HVM_PARAM_VM86_TSS, virt_to_phys(tss)); + hvm_param_set(HVM_PARAM_VM86_TSS_SIZE, TSS_SIZE); printf("vm86 TSS at %08lx\n", virt_to_phys(tss)); +#undef TSS_SIZE } static void apic_setup(void) --- a/tools/libxc/xc_sr_save_x86_hvm.c +++ b/tools/libxc/xc_sr_save_x86_hvm.c @@ -68,6 +68,7 @@ static int write_hvm_params(struct xc_sr HVM_PARAM_MONITOR_RING_PFN, HVM_PARAM_SHARING_RING_PFN, HVM_PARAM_VM86_TSS, + HVM_PARAM_VM86_TSS_SIZE, HVM_PARAM_CONSOLE_PFN, HVM_PARAM_ACPI_IOPORTS_LOCATION, HVM_PARAM_VIRIDIAN, --- a/xen/arch/x86/hvm/hvm.c +++ b/xen/arch/x86/hvm/hvm.c @@ -2850,6 +2850,43 @@ static int hvm_load_segment_selector( return 1; } +struct tss32 { + uint16_t back_link, :16; + uint32_t esp0; + uint16_t ss0, :16; + uint32_t esp1; + uint16_t ss1, :16; + uint32_t esp2; + uint16_t ss2, :16; + uint32_t cr3, eip, eflags, eax, ecx, edx, ebx, esp, ebp, esi, edi; + uint16_t es, :16, cs, :16, ss, :16, ds, :16, fs, :16, gs, :16, ldt, :16; + uint16_t trace /* :1 */, iomap; +}; + +void hvm_prepare_vm86_tss(struct vcpu *v, uint32_t base, uint32_t limit) +{ + /* + * If the provided area is large enough to cover at least the ISA port + * range, keep the bitmaps outside the base structure. For rather small + * areas (namely relevant for guests having been migrated from older + * Xen versions), maximize interrupt vector and port coverage by pointing + * the I/O bitmap at 0x20 (which puts the interrupt redirection bitmap + * right at zero), accepting accesses to port 0x235 (represented by bit 5 + * of byte 0x46) to trigger #GP (which will simply result in the access + * being handled by the emulator via a slightly different path than it + * would be anyway). Be sure to include one extra byte at the end of the + * I/O bitmap (hence the missing "- 1" in the comparison is not an + * off-by-one mistake), which we deliberately don't fill with all ones. + */ + uint16_t iomap = (limit >= sizeof(struct tss32) + (0x100 / 8) + (0x400 / 8) + ? sizeof(struct tss32) : 0) + (0x100 / 8); + + ASSERT(limit >= sizeof(struct tss32) - 1); + hvm_copy_to_guest_phys(base, NULL, limit + 1, v); + hvm_copy_to_guest_phys(base + offsetof(struct tss32, iomap), + &iomap, sizeof(iomap), v); +} + void hvm_task_switch( uint16_t tss_sel, enum hvm_task_switch_reason taskswitch_reason, int32_t errcode) @@ -2862,18 +2899,7 @@ void hvm_task_switch( unsigned int eflags; pagefault_info_t pfinfo; int exn_raised, rc; - struct { - u16 back_link,__blh; - u32 esp0; - u16 ss0, _0; - u32 esp1; - u16 ss1, _1; - u32 esp2; - u16 ss2, _2; - u32 cr3, eip, eflags, eax, ecx, edx, ebx, esp, ebp, esi, edi; - u16 es, _3, cs, _4, ss, _5, ds, _6, fs, _7, gs, _8, ldt, _9; - u16 trace, iomap; - } tss; + struct tss32 tss; hvm_get_segment_register(v, x86_seg_gdtr, &gdt); hvm_get_segment_register(v, x86_seg_tr, &prev_tr); @@ -4276,6 +4302,7 @@ static int hvm_allow_set_param(struct do /* The following parameters can be set by the guest. */ case HVM_PARAM_CALLBACK_IRQ: case HVM_PARAM_VM86_TSS: + case HVM_PARAM_VM86_TSS_SIZE: case HVM_PARAM_ACPI_IOPORTS_LOCATION: case HVM_PARAM_VM_GENERATION_ID_ADDR: case HVM_PARAM_STORE_EVTCHN: @@ -4484,6 +4511,31 @@ static int hvmop_set_param( } d->arch.x87_fip_width = a.value; break; + + case HVM_PARAM_VM86_TSS: + /* Hardware would silently truncate high bits. */ + if ( a.value != (uint32_t)a.value ) + { + if ( d == curr_d ) + domain_crash(d); + rc = -EINVAL; + } + break; + + case HVM_PARAM_VM86_TSS_SIZE: + if ( a.value < sizeof(struct tss32) ) + { + if ( d == curr_d ) + domain_crash(d); + rc = -EINVAL; + } + /* + * Cap at the theoretically useful maximum (base structure plus + * 256 bits interrupt redirection bitmap + 64k bits I/O bitmap). + */ + else if ( a.value > sizeof(struct tss32) + (0x100 / 8) + (0x10000 / 8) ) + a.value = sizeof(struct tss32) + (0x100 / 8) + (0x10000 / 8); + break; } if ( rc != 0 ) @@ -4513,6 +4565,7 @@ static int hvm_allow_get_param(struct do /* The following parameters can be read by the guest. */ case HVM_PARAM_CALLBACK_IRQ: case HVM_PARAM_VM86_TSS: + case HVM_PARAM_VM86_TSS_SIZE: case HVM_PARAM_ACPI_IOPORTS_LOCATION: case HVM_PARAM_VM_GENERATION_ID_ADDR: case HVM_PARAM_STORE_PFN: --- a/xen/arch/x86/hvm/vmx/vmx.c +++ b/xen/arch/x86/hvm/vmx/vmx.c @@ -1120,12 +1120,20 @@ static void vmx_set_segment_register(str if ( seg == x86_seg_tr ) { - if ( v->domain->arch.hvm_domain.params[HVM_PARAM_VM86_TSS] ) + const struct domain *d = v->domain; + + if ( d->arch.hvm_domain.params[HVM_PARAM_VM86_TSS] ) { sel = 0; attr = vm86_tr_attr; - limit = 0xff; - base = v->domain->arch.hvm_domain.params[HVM_PARAM_VM86_TSS]; + /* + * Old hvmloader binaries hardcode the size to 128 bytes, + * without setting HVM_PARAM_VM86_TSS_SIZE. + */ + limit = (d->arch.hvm_domain.params[HVM_PARAM_VM86_TSS_SIZE] + ?: 0x80) - 1; + base = d->arch.hvm_domain.params[HVM_PARAM_VM86_TSS]; + hvm_prepare_vm86_tss(v, base, limit); v->arch.hvm_vmx.vm86_segment_mask &= ~(1u << seg); } else --- a/xen/include/asm-x86/hvm/support.h +++ b/xen/include/asm-x86/hvm/support.h @@ -111,6 +111,8 @@ int hvm_do_hypercall(struct cpu_user_reg void hvm_hlt(unsigned int eflags); void hvm_triple_fault(void); +void hvm_prepare_vm86_tss(struct vcpu *v, uint32_t base, uint32_t limit); + void hvm_rdtsc_intercept(struct cpu_user_regs *regs); int __must_check hvm_handle_xsetbv(u32 index, u64 new_bv); --- a/xen/include/public/hvm/params.h +++ b/xen/include/public/hvm/params.h @@ -253,6 +253,9 @@ */ #define HVM_PARAM_X87_FIP_WIDTH 36 -#define HVM_NR_PARAMS 37 +/* Size of TSS used on Intel when CR0.PE=0. */ +#define HVM_PARAM_VM86_TSS_SIZE 37 + +#define HVM_NR_PARAMS 38 #endif /* __XEN_PUBLIC_HVM_PARAMS_H__ */