Re: [Xen-devel] [PATCH 3/4 V2] X86: MPX IA32_BNDCFGS msr handle

["Liu, Jinsong" <jinsong.liu@xxxxxxxxx>]

Jan Beulich wrote:
>>>> On 25.11.13 at 09:25, "Liu, Jinsong" <jinsong.liu@xxxxxxxxx> wrote:
>> +static bool_t bndcfgs_invalid(u64 msr_content)
>> +{
>> +    /* BNDCFGS MSR reserved bits (11 ~ 2) must be zero */
>> +    if ( msr_content & 0xffc )
>> +        return 1;
>> +
>> +    /* Canonical address reserved bits must be zero */
>> +    if ( hvm_long_mode_enabled(current) )
>> +        /* 48b linear address for x86_64 guest */
>> +        return !!(msr_content & (~(u64)0 << 48) );
> 
> !is_canonical_address()

Not simply is_canonical_address() check, per MPX doc 9.3.5.1,
It will #GP if canonical address reserved bits (must be zero) check fails

> 
>> +    else
>> +        /* 32b linear address for x86_32 (include PAE) guest */
>> +        return !!(msr_content & (~(u64)0 << 32) );
> 
> !!(msr_content >> 32)
> 
>> @@ -3010,6 +3025,12 @@ int hvm_msr_read_intercept(unsigned int msr,
>>          uint64_t *msr_content) hvm_get_guest_pat(v, msr_content);
>>          break;
>> 
>> +    case MSR_IA32_BNDCFGS:
>> +        if ( !cpu_has_mpx )
> 
> Wasn't it you who started to properly use hvm_cpuid() for cases like
> this? We're not after host capabilities here, but after what the guest
> is supposed to (not) use.

When malicious or flawed guest access reserved or unimplemented msr, I think 
inject #GP is better than silently ignore?

> 
>> @@ -955,6 +956,34 @@ static int construct_vmcs(struct vcpu *v)
>>          vmx_disable_intercept_for_msr(v, MSR_IA32_SYSENTER_EIP,
>>          MSR_TYPE_R | MSR_TYPE_W); if ( paging_mode_hap(d) &&
>>              (!iommu_enabled || iommu_snoop) )
>> vmx_disable_intercept_for_msr(v, MSR_IA32_CR_PAT, MSR_TYPE_R |
>> MSR_TYPE_W); + +        if ( cpu_has_mpx ) +        {
>> +            /*
>> +             * When MPX supported, a new guest-state field for
>> IA32_BNDCFGS +             * is added to the VMCS. In addition, two
>> new controls are added: +             * - a VM-exit control called
>> "clear BNDCFGS" +             * - a VM-entry control called "load
>> BNDCFGS." +             * VM exits always save IA32_BNDCFGS into
>> BNDCFGS field of VMCS. +             */ +            if (
>> likely((vmexit_ctl & VM_EXIT_CLEAR_BNDCFGS) && +                    
>> (vmentry_ctl & VM_ENTRY_LOAD_BNDCFGS)) ) +            { +           
>> vmx_disable_intercept_for_msr(v, MSR_IA32_BNDCFGS, +                
>> MSR_TYPE_R | MSR_TYPE_W); +            } +            /* Unlikely,
>> just in case buggy VMX ucode */ +            else +            {
>> +                int ret;
>> +                ret = vmx_add_guest_msr(MSR_IA32_BNDCFGS); +       
>> if ( ret ) +                    return ret;
>> +                ret = vmx_add_host_load_msr(MSR_IA32_BNDCFGS); +   
>> if ( ret ) +                    return ret;
> 
> You can't just return here - a vmx_vmcs_exit() is needed on any
> exit path in the middle of this function.
> 

Yes.

Thanks,
Jinsong

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