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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> 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?

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

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

> +     *
> +     * 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()

And why is this HAP-specific?

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

Jan


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