Re: [Xen-devel] [PATCH] x86/VMX: sanitize VM86 TSS handling

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 13/02/17 13:19, Jan Beulich wrote:
> 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 <jbeulich@xxxxxxxx>
> ---
> 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.

The only case we need worry about is if the guest manually writes to the
area covered by the vm86 tss.  I think, looking at the logic, that we
properly restore the old %tr when re-entering protected mode, so we
aren't at risk of having the vm86 tss on the leaving-side of a hardware
task switch.

We have lasted thus far without, but given the issues recently
identified, the only safe conclusion to be drawn is that this
functionality hasn't been used in anger.

For sensible performance, we need to keep the IO bitmap clear to avoid
taking the #GP emulation path.

For correct behaviour, we need the entire software bitmap to be 0.

As this functionality exists only for first-gen VT-x lacking
unrestricted_guest, I can't claim to care too much about the performance
impact.

> --- 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);

This should be linear rather than phys.

In practice it will only make a difference if the guest manages to
corrupt its IDENT_PT (which is why I suggested that we move the IDENT_PT
entirely outside of the guest control), but in such a case, we will
re-enter the guest with the vm86 tss pointing to something other than
what we have just initialised.

> +}
> +
>  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. */

Does it?  I'd have thought it would be a vmentry failure.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel