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

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

Jan Beulich wrote:
>>>> "Liu, Jinsong" <jinsong.liu@xxxxxxxxx> 11/26/13 3:12 PM >>>
>> Jan Beulich wrote:
>>>>>> On 26.11.13 at 11:11, "Liu, Jinsong" <jinsong.liu@xxxxxxxxx>
>>>>>> wrote: 
>>>> 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
>>> 
>>> You're checking the reserved bits (2...11) above. For the address
>>> portion it says "The base of bound directory is a 4K page aligned
>>> linear address, and is always in canonical form. Any load into
>>> BNDCFGx (XRSTOR or WRMSR) ensures that the highest implemented bit
>>> of 
>>> the linear address is sign extended to guarantee the canonicality
>>> of this address", which to me means _if_ you check anything here,
>>> then you want to check the address for being canonical.
>>> 
>>>>>> +    else
>>>>>> +        /* 32b linear address for x86_32 (include PAE) guest */
>>>>>> +        return !!(msr_content & (~(u64)0 << 32) );
>>>>> 
>>>>> !!(msr_content >> 32)
>> 
>> If so, canonical check for x86-32 (and pae) -- bit63~32 should not
>> always be zero, but is sign extension of bit 31? 
> 
> Why? Everywhere else 32-bit addresses get zero extended when a 64-bit
> value is needed.
> 
> But first of all the question needs to be answered whether this
> checking is necessary at all - the documentation to me reads as if
> the hardware would not fault on non-canonical values, but simply make
> them canonical. 

Re-read doc, seems your understanding is right.
So we check bit11~2 and inject #GP if check fail, and emulate making canonical 
address:
* for x86-64 guest, sign extend bit47
* for x86-32 guest, bit63~32 set all 0's

>>>>>> @@ -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?
>>> 
>>> I didn't say "silently ignore"; in fact, I asked you to _tighten_
>>> the check (hvm_cpuid() shouldn't return the respective flag set when
>>> !cpu_has_mpx).
>> 
>> Not quite clear your meaning here, hvm_cpuid will not return flag
>> when !cpu_has_mpx. 
> 
> In your patch you check the hardware capability, but you ought to
> check whether the guest knows the feature to be available. The
> feature will obviously not show as available if the hardware
> capability is not there. 

But the check !cpu_has_mpx has implicitly meant guest will not know this 
feature -- if h/w has not mpx cability, xen will not expose it to guest. 
Anything I'm missing here?

Thanks,
Jinsong



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