Re: [Xen-devel] [PATCH] xen/x86: ensure copying to L1 guest in update_runstate_area()

[Haozhong Zhang <haozhong.zhang@xxxxxxxxx>]

On 02/21/17 02:15 -0700, Jan Beulich wrote:
> >>> On 21.02.17 at 03:11, <haozhong.zhang@xxxxxxxxx> wrote:
> > For a HVM domain, if a vcpu is in the nested guest mode,
> > __raw_copy_to_guest() and __copy_to_guest() used by
> > update_runstate_area() will copy data to L2 guest other than L1 guest.
> > 
> > Besides copying to the wrong address, this bug also causes crash in
> > the code path:
> >     context_switch(prev, next)
> >       _update_runstate_area(next)
> >         update_runstate_area(next)
> >       __raw_copy_to_guest(...)
> >         ...
> >           __hvm_copy(...)
> >             paging_gva_to_gfn(...)
> >               nestedhap_walk_L1_p2m(...)
> >                 nvmx_hap_walk_L1_p2m(..)
> >                   vmx_vmcs_enter(v) [ v = next ]
> >                     vmx_vmcs_try_enter(v) [ v = next ]
> >                       if ( likely(v == current) )
> >                               return v->arch.hvm_vmx.vmcs_pa == 
> > this_cpu(current_vmcs);
> > vmx_vmcs_try_enter() will fail and trigger the assert in
> > vmx_vmcs_enter(), if vcpu 'next' is in the nested guest mode and is
> > being scheduled to another CPU.
> > 
> > This commit temporally clears the nested guest flag before all
> > __raw_copy_to_guest() and __copy_to_guest() in update_runstate_area(),
> > and restores the flag after those guest copy operations.
> 
> Nice catch, but doesn't the same apply to
> update_secondary_system_time() then?
>

Yes, I'll apply the same change to update_secondary_system_time().

> > @@ -1931,10 +1932,29 @@ bool_t update_runstate_area(struct vcpu *v)
> >      bool_t rc;
> >      smap_check_policy_t smap_policy;
> >      void __user *guest_handle = NULL;
> > +    bool nested_guest_mode = 0;
> 
> false

will change

> 
> >      if ( guest_handle_is_null(runstate_guest(v)) )
> >          return 1;
> >  
> > +    /*
> > +     * Must be before all following __raw_copy_to_guest() and 
> > __copy_to_guest().
> > +     *
> > +     * Otherwise, if 'v' is in the nested guest mode, paging_gva_to_gfn() 
> > called
> > +     * from __raw_copy_to_guest() and __copy_to_guest() will treat the 
> > target
> > +     * address as L2 gva, and __raw_copy_to_guest() and __copy_to_guest() 
> > will
> > +     * consequently copy runstate to L2 guest other than L1 guest.
> 
> ... rather than ...

ditto

> 
> > +     *
> > +     * Therefore, we clear the nested guest flag before 
> > __raw_copy_to_guest()
> > +     * and __copy_to_guest(), and restore the flag after all guest copy.
> > +     */
> > +    if ( is_hvm_vcpu(v) && paging_mode_hap(v->domain) )
> 
> has_hvm_container_vcpu()

Nested HVM is only available to HVM domain, so I think is_hvm_vcpu(v) is enough.

> 
> And why is this HAP-specific?
>

IIUC, nested HVM relies on HAP.

> > @@ -1971,6 +1991,9 @@ bool_t update_runstate_area(struct vcpu *v)
> >  
> >      smap_policy_change(v, smap_policy);
> >  
> > +    if ( nested_guest_mode )
> 
> Can we have an unlikely() here please?

will add

Thanks,
Haozhong

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