| Message ID | 20211011143702.1786568-5-eesposit@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: nSVM: avoid TOC/TOU race when checking vmcb12 | expand |
On 11/10/21 16:36, Emanuele Giuseppe Esposito wrote: > + > +out_free_save: > + memset(&svm->nested.save, 0, sizeof(struct vmcb_save_area_cached)); > + This memset is not strictly necessary, is it? (Same for out_free_ctl later on). Paolo
On 22/10/2021 09:14, Paolo Bonzini wrote: > On 11/10/21 16:36, Emanuele Giuseppe Esposito wrote: >> + >> +out_free_save: >> + memset(&svm->nested.save, 0, sizeof(struct vmcb_save_area_cached)); >> + > > This memset is not strictly necessary, is it? (Same for out_free_ctl > later on). > It was just to keep the struct clean in case of error, but you are right, it can be removed. Thank you, Emanuele
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
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; /* * 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)); + out_free: kfree(save); kfree(ctl);
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(-)