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

["Jan Beulich" <jbeulich@xxxxxxxx>]

>>> "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.

>>>>> @@ -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.

Jan


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