Re: [PATCH v3 4/8] nSVM: use vmcb_save_area_cached in nested_vmcb_valid_sregs()

[Maxim Levitsky <mlevitsk@redhat.com>]

On Mon, 2021-10-11 at 10:36 -0400, Emanuele Giuseppe Esposito wrote:
> Now that struct vmcb_save_area_cached contains the required
> vmcb fields values (done in nested_load_save_from_vmcb12()),
> check them to see if they are correct in nested_vmcb_valid_sregs().
> 
> Since we are always checking for the nested struct, it is enough
> to have only the vcpu as parameter.
> 
> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
> ---
>  arch/x86/kvm/svm/nested.c | 19 ++++++++++++-------
>  1 file changed, 12 insertions(+), 7 deletions(-)
> 
> diff --git a/arch/x86/kvm/svm/nested.c b/arch/x86/kvm/svm/nested.c
> index f6030a202bc5..d07cd4b88acd 100644
> --- a/arch/x86/kvm/svm/nested.c
> +++ b/arch/x86/kvm/svm/nested.c
> @@ -230,9 +230,10 @@ static bool nested_vmcb_check_controls(struct kvm_vcpu *vcpu,
>  }
>  
>  /* Common checks that apply to both L1 and L2 state.  */
> -static bool nested_vmcb_valid_sregs(struct kvm_vcpu *vcpu,
> -				    struct vmcb_save_area *save)
> +static bool nested_vmcb_valid_sregs(struct kvm_vcpu *vcpu)
>  {
> +	struct vcpu_svm *svm = to_svm(vcpu);
> +	struct vmcb_save_area_cached *save = &svm->nested.save;
>  	/*
>  	 * FIXME: these should be done after copying the fields,
>  	 * to avoid TOC/TOU races.  For these save area checks
> @@ -658,7 +659,7 @@ int nested_svm_vmrun(struct kvm_vcpu *vcpu)
>  	nested_copy_vmcb_control_to_cache(svm, &vmcb12->control);
>  	nested_copy_vmcb_save_to_cache(svm, &vmcb12->save);
>  
> -	if (!nested_vmcb_valid_sregs(vcpu, &vmcb12->save) ||
> +	if (!nested_vmcb_valid_sregs(vcpu) ||
>  	    !nested_vmcb_check_controls(vcpu, &svm->nested.ctl)) {
>  		vmcb12->control.exit_code    = SVM_EXIT_ERR;
>  		vmcb12->control.exit_code_hi = 0;
> @@ -1355,11 +1356,12 @@ static int svm_set_nested_state(struct kvm_vcpu *vcpu,
>  	 * Validate host state saved from before VMRUN (see
>  	 * nested_svm_check_permissions).
>  	 */
> +	nested_copy_vmcb_save_to_cache(svm, save);


>  	if (!(save->cr0 & X86_CR0_PG) ||
>  	    !(save->cr0 & X86_CR0_PE) ||
>  	    (save->rflags & X86_EFLAGS_VM) ||
> -	    !nested_vmcb_valid_sregs(vcpu, save))
> -		goto out_free;
> +	    !nested_vmcb_valid_sregs(vcpu))
> +		goto out_free_save;

The two changes from above can't be done like that sadly.

We cache only vmcb12 because it comes from the guest which is untrusted.

Here we validate the L1's saved host state which is (once again,
SVM nested state is confused), the 'save' variable.
That state is not guest controlled, but here we validate it
to avoid trusting the KVM_SET_NESTED_STATE caller.

I guess we can copy this to an extra 'struct vmcb_save_area_cached' on stack (this struct is small),
and then pass its address to nested_vmcb_valid_sregs (or better to __nested_vmcb_valid_sregs,
so that nested_vmcb_valid_sregs could still take one parameter, but delegate
its work to __nested_vmcb_valid_sregs with two parameters.

>  
>  	/*
>  	 * While the nested guest CR3 is already checked and set by
> @@ -1371,7 +1373,7 @@ static int svm_set_nested_state(struct kvm_vcpu *vcpu,
>  	ret = nested_svm_load_cr3(&svm->vcpu, vcpu->arch.cr3,
>  				  nested_npt_enabled(svm), false);
>  	if (WARN_ON_ONCE(ret))
> -		goto out_free;
> +		goto out_free_save;
>  
>  
>  	/*
> @@ -1395,12 +1397,15 @@ static int svm_set_nested_state(struct kvm_vcpu *vcpu,
>  
>  	svm_copy_vmrun_state(&svm->vmcb01.ptr->save, save);
>  	nested_copy_vmcb_control_to_cache(svm, ctl);
> -	nested_copy_vmcb_save_to_cache(svm, save);
>  
>  	svm_switch_vmcb(svm, &svm->nested.vmcb02);
>  	nested_vmcb02_prepare_control(svm);
>  	kvm_make_request(KVM_REQ_GET_NESTED_STATE_PAGES, vcpu);
>  	ret = 0;
> +
> +out_free_save:
> +	memset(&svm->nested.save, 0, sizeof(struct vmcb_save_area_cached));
> +

This won't be needed if we don't touch svm->nested.save.

>  out_free:
>  	kfree(save);
>  	kfree(ctl);


Best regards,
	Maxim Levitsky