Re: [Xen-devel] [PATCH 08/15] x86/hvm: Reposition the modification of raw segment data from the VMCB/VMCS

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 23/11/16 19:01, Boris Ostrovsky wrote:
> On 11/23/2016 10:38 AM, Andrew Cooper wrote:
>> Intel VT-x and AMD SVM provide access to the full segment descriptor cache 
>> via
>> fields in the VMCB/VMCS.  However, the bits which are actually checked by
>> hardware and preserved across vmentry/exit are inconsistent, and the vendor
>> accessor functions perform inconsistent modification to the raw values.
>>
>> Convert {svm,vmx}_{get,set}_segment_register() into raw accessors, and alter
>> hvm_{get,set}_segment_register() to cook the values consistently.  This 
>> allows
>> the common emulation code to better rely on finding architecturally-expected
>> values.
>>
>> This does cause some functional changes because of the modifications being
>> applied uniformly.  A side effect of this fixes latent bugs where
>> vmx_set_segment_register() didn't correctly fix up .G for segments, and
>> inconsistent fixing up of the GDTR/IDTR limits.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
>> ---
>> CC: Jan Beulich <JBeulich@xxxxxxxx>
>> CC: Jun Nakajima <jun.nakajima@xxxxxxxxx>
>> CC: Kevin Tian <kevin.tian@xxxxxxxxx>
>> CC: Boris Ostrovsky <boris.ostrovsky@xxxxxxxxxx>
>> CC: Suravee Suthikulpanit <suravee.suthikulpanit@xxxxxxx>
>> ---
>>  xen/arch/x86/hvm/hvm.c        | 151 
>> ++++++++++++++++++++++++++++++++++++++++++
>>  xen/arch/x86/hvm/svm/svm.c    |  20 +-----
>>  xen/arch/x86/hvm/vmx/vmx.c    |   6 +-
>>  xen/include/asm-x86/desc.h    |   6 ++
>>  xen/include/asm-x86/hvm/hvm.h |  17 ++---
>>  5 files changed, 164 insertions(+), 36 deletions(-)
>>
>> diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
>> index ef83100..804cd88 100644
>> --- a/xen/arch/x86/hvm/hvm.c
>> +++ b/xen/arch/x86/hvm/hvm.c
>> @@ -6051,6 +6051,157 @@ void hvm_domain_soft_reset(struct domain *d)
>>  }
>>  
>>  /*
>> + * Segment caches in VMCB/VMCS are inconsistent about which bits are 
>> checked,
>> + * important, and preserved across vmentry/exit.  Cook the values to make 
>> them
>> + * closer to what is architecturally expected from entries in the segment
>> + * cache.
>> + */
>> +void hvm_get_segment_register(struct vcpu *v, enum x86_segment seg,
>> +                              struct segment_register *reg)
>> +{
>> +    hvm_funcs.get_segment_register(v, seg, reg);
>> +
>> +    switch ( seg )
>> +    {
>> +    case x86_seg_ss:
>> +        /* SVM may retain %ss.DB when %ss is loaded with a NULL selector. */
>> +        if ( !reg->attr.fields.p )
>> +            reg->attr.fields.db = 0;
> Removed SVM code relied on type being zero to indicate a NULL segment.
> Looking at P bit is the correct way and I think it's worth mentioning
> this in the commit message.

Oh yes.  As far as I can tell, that was simply broken before.

>
>
>> +}
>> +
>> +void hvm_set_segment_register(struct vcpu *v, enum x86_segment seg,
>> +                              struct segment_register *reg)
>> +{
>> +    /* Set G to match the limit field.  VT-x cares, while SVM doesn't. */
>> +    if ( reg->attr.fields.p )
>> +        reg->attr.fields.g = !!(reg->limit >> 20);
>> +
>> +    switch ( seg )
>> +    {
>> +    case x86_seg_cs:
>> +        ASSERT(reg->attr.fields.p);                  /* Usable. */
>> +        ASSERT(reg->attr.fields.s);                  /* User segment. */
>> +        ASSERT((reg->base >> 32) == 0);              /* Upper bits clear. */
>> +        break;
>> +
>> +    case x86_seg_ss:
>> +        if ( reg->attr.fields.p )
>> +        {
>> +            ASSERT(reg->attr.fields.s);              /* User segment. */
>> +            ASSERT(!(reg->attr.fields.type & 0x8));  /* Data segment. */
>> +            ASSERT(reg->attr.fields.type & 0x2);     /* Writeable. */
>> +            ASSERT((reg->base >> 32) == 0);          /* Upper bits clear. */
>> +        }
>> +        break;
> SVM requires attributes of any NULL segment to be zero.

Where is this claim made?  Vol2 recommends that the VMM clear all
attributes, but the wording of the previous paragraph suggests that the
attributes would be ignored in this case.   The %ss bug, and some
experimentation on my behalf also indicate that they are ignored.

> I don't know about Intel but if it's the same then should we ASSERT this as 
> well?

On Intel if unusable is set, all other bits are ignored.

However, the behaviour of both Intel and AMD is to occasionally set
upper attribute bits.  At some point I intend to make emulated segment
loading match d->arch.vendor's behaviour, at which point such an
ASSERT() would definitely trip.

~Andrew

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