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

["Jan Beulich" <JBeulich@xxxxxxxx>]

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

Jan


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