Re: [Xen-devel] [PATCH] XSA-60 security hole: cr0.cd handling

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 11.10.13 at 20:25, "Liu, Jinsong" <jinsong.liu@xxxxxxxxx> wrote:
> +void hvm_handle_cd_traditional(struct vcpu *v, unsigned long value)
> +{
> +    if ( (value & X86_CR0_CD) && !(value & X86_CR0_NW) )
> +    {
> +        /* Entering no fill cache mode. */
> +        spin_lock(&v->domain->arch.hvm_domain.uc_lock);
> +        v->arch.hvm_vcpu.cache_mode = NO_FILL_CACHE_MODE;
> +
> +        if ( !v->domain->arch.hvm_domain.is_in_uc_mode )
> +        {
> +            /* Flush physical caches. */
> +            on_each_cpu(local_flush_cache, NULL, 1);
> +            hvm_set_uc_mode(v, 1);

So you continue to use the problematic function, and you also
don't remove the problematic code from it's VMX backend - what
am I missing here? If there was no room for getting here with
!paging_mode_hap(), the problematic code should be removed
from vmx_set_uc_mode(). And if we still can get here with
!paging_mode_hap(), then what's the purpose of the patch?

> +    /*
> +     * When cr0.cd setting
> +     * 1. For guest w/o VT-d, and for guest with VT-d but snooped, Xen need 
> not
> +     * do anything, since hardware snoop mechanism has ensured cache 
> coherency;
> +     * 2. For guest with VT-d but non-snooped, cache coherency cannot be
> +     * guaranteed by h/w so need emulate UC memory type to guest.
> +     */
> +    if ( ((value ^ old_value) & X86_CR0_CD) &&
> +           has_arch_mmios(v->domain) &&

has_arch_mmios() has been wrong (too weak) here originally, so I
strongly suggest replacing it here as you modify this logic anyway:
This check must consider not only non-empty iomem_caps, but also
non-empty ioport_caps.

> +static void vmx_handle_cd(struct vcpu *v, unsigned long value)
> +{
> +    if ( !paging_mode_hap(v->domain) ||
> +         unlikely(!cpu_has_vmx_pat) )    /* Too old for EPT, just for safe */
> +    {
> +        /*
> +         * For shadow, 'load IA32_PAT' VM-entry control is 0, so it cannot
> +         * set guest memory type as UC via IA32_PAT. Xen drop all shadows
> +         * so that any new ones would be created on demand.
> +         */
> +        hvm_handle_cd_traditional(v, value);
> +    }
> +    else
> +    {
> +        u64 *pat = &v->arch.hvm_vcpu.pat_cr;
> +
> +        if ( value & X86_CR0_CD )
> +        {
> +            /*
> +             * For EPT, set guest IA32_PAT fields as UC so that guest
> +             * memory type are all UC.
> +             */
> +            u64 uc_pat =
> +                ((uint64_t)PAT_TYPE_UNCACHABLE)       |       /* PAT0 */
> +                ((uint64_t)PAT_TYPE_UNCACHABLE << 8)  |       /* PAT1 */
> +                ((uint64_t)PAT_TYPE_UNCACHABLE << 16) |       /* PAT2 */
> +                ((uint64_t)PAT_TYPE_UNCACHABLE << 24) |       /* PAT3 */
> +                ((uint64_t)PAT_TYPE_UNCACHABLE << 32) |       /* PAT4 */
> +                ((uint64_t)PAT_TYPE_UNCACHABLE << 40) |       /* PAT5 */
> +                ((uint64_t)PAT_TYPE_UNCACHABLE << 48) |       /* PAT6 */
> +                ((uint64_t)PAT_TYPE_UNCACHABLE << 56);        /* PAT7 */
> +
> +            vmx_get_guest_pat(v, pat);
> +            vmx_set_guest_pat(v, uc_pat);
> +
> +            wbinvd();
> +            v->arch.hvm_vcpu.cache_mode = NO_FILL_CACHE_MODE;
> +        }
> +        else
> +        {
> +            vmx_set_guest_pat(v, *pat);
> +            v->arch.hvm_vcpu.cache_mode = NORMAL_CACHE_MODE;
> +        }

All this is done only on the current vCPU, and I don't see any
mechanism by which the other vCPU-s of the domain would
similarly get their PAT overridden (yet the whole domain gets
transitioned to UC mode). This minimally needs more cleanup (i.e.
it should be uniformly just the local vCPU or uniformly the whole
domain that is affected here).

And if going the local-vCPU-only route you need to make clear
(via code comment I would think) that this is safe to do when
two of the guest's vCPU-s with different CR0.CD settings run
on two hyperthreads on the same core.

Jan


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