| Message ID | 20211103140527.752797-1-eesposit@redhat.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | KVM: nSVM: avoid TOC/TOU race when checking vmcb12 | expand |
On 11/3/21 15:05, Emanuele Giuseppe Esposito wrote: > Currently there is a TOC/TOU race between the check of vmcb12's > efer, cr0 and cr4 registers and the later save of their values in > svm_set_*, because the guest could modify the values in the meanwhile. > > To solve this issue, this series introduces and uses svm->nested.save > structure in enter_svm_guest_mode to save the current value of efer, > cr0 and cr4 and later use these to set the vcpu->arch.* state. > > Similarly, svm->nested.ctl contains fields that are not used, so having > a full vmcb_control_area means passing uninitialized fields. > > Patches 1,3 and 8 take care of renaming and refactoring code. > Patches 2 and 6 introduce respectively vmcb_ctrl_area_cached and > vmcb_save_area_cached. > Patches 4 and 5 use vmcb_save_area_cached to avoid TOC/TOU, and patch > 7 uses vmcb_ctrl_area_cached. > > Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com> Queued (for 5.17). I changed the helper functions to have a "__" prefix since that's more common in Linux. Paolo > > --- > v5: > * rebased on kvm/queue branch > > v4: > * introduce _* helpers (_nested_vmcb_check_save, > _nested_copy_vmcb_control_to_cache, _nested_copy_vmcb_save_to_cache) > that take care of additional parameters. > * svm_set_nested_state: introduce {save, ctl}_cached variables > to not pollute svm->nested.{save,ctl} state, especially if the > check fails. remove also unnecessary memset added in previous versions. > * svm_get_nested_state: change stack variable ctl introduced in this series > into a pointer that will be zeroed and freed after it has been copied to user > > v3: > * merge this series with "KVM: nSVM: use vmcb_ctrl_area_cached instead > of vmcb_control_area in nested state" > * rename "nested_load_save_from_vmcb12" in > "nested_copy_vmcb_save_to_cache" > * rename "nested_load_control_from_vmcb12" in > "nested_copy_vmcb_control_to_cache" > * change check functions (nested_vmcb_valid_sregs and nested_vmcb_valid_sregs) > to accept only the vcpu parameter, since we only check > nested state now > * rename "vmcb_is_intercept_cached" in "vmcb12_is_intercept" > and duplicate the implementation instead of calling vmcb_is_intercept > > v2: > * svm->nested.save is a separate struct vmcb_save_area_cached, > and not vmcb_save_area. > * update also vmcb02->cr3 with svm->nested.save.cr3 > > RFC: > * use svm->nested.save instead of local variables. > * not dependent anymore from "KVM: nSVM: remove useless kvm_clear_*_queue" > * simplified patches, we just use the struct and not move the check > nearer to the TOU. > > Emanuele Giuseppe Esposito (7): > KVM: nSVM: move nested_vmcb_check_cr3_cr4 logic in > nested_vmcb_valid_sregs > nSVM: introduce smv->nested.save to cache save area fields > nSVM: rename nested_load_control_from_vmcb12 in > nested_copy_vmcb_control_to_cache > nSVM: use vmcb_save_area_cached in nested_vmcb_valid_sregs() > nSVM: use svm->nested.save to load vmcb12 registers and avoid TOC/TOU > races > nSVM: introduce struct vmcb_ctrl_area_cached > nSVM: use vmcb_ctrl_area_cached instead of vmcb_control_area in struct > svm_nested_state > > arch/x86/kvm/svm/nested.c | 250 ++++++++++++++++++++++++-------------- > arch/x86/kvm/svm/svm.c | 7 +- > arch/x86/kvm/svm/svm.h | 59 ++++++++- > 3 files changed, 213 insertions(+), 103 deletions(-) >
Currently there is a TOC/TOU race between the check of vmcb12's efer, cr0 and cr4 registers and the later save of their values in svm_set_*, because the guest could modify the values in the meanwhile. To solve this issue, this series introduces and uses svm->nested.save structure in enter_svm_guest_mode to save the current value of efer, cr0 and cr4 and later use these to set the vcpu->arch.* state. Similarly, svm->nested.ctl contains fields that are not used, so having a full vmcb_control_area means passing uninitialized fields. Patches 1,3 and 8 take care of renaming and refactoring code. Patches 2 and 6 introduce respectively vmcb_ctrl_area_cached and vmcb_save_area_cached. Patches 4 and 5 use vmcb_save_area_cached to avoid TOC/TOU, and patch 7 uses vmcb_ctrl_area_cached. Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com> --- v5: * rebased on kvm/queue branch v4: * introduce _* helpers (_nested_vmcb_check_save, _nested_copy_vmcb_control_to_cache, _nested_copy_vmcb_save_to_cache) that take care of additional parameters. * svm_set_nested_state: introduce {save, ctl}_cached variables to not pollute svm->nested.{save,ctl} state, especially if the check fails. remove also unnecessary memset added in previous versions. * svm_get_nested_state: change stack variable ctl introduced in this series into a pointer that will be zeroed and freed after it has been copied to user v3: * merge this series with "KVM: nSVM: use vmcb_ctrl_area_cached instead of vmcb_control_area in nested state" * rename "nested_load_save_from_vmcb12" in "nested_copy_vmcb_save_to_cache" * rename "nested_load_control_from_vmcb12" in "nested_copy_vmcb_control_to_cache" * change check functions (nested_vmcb_valid_sregs and nested_vmcb_valid_sregs) to accept only the vcpu parameter, since we only check nested state now * rename "vmcb_is_intercept_cached" in "vmcb12_is_intercept" and duplicate the implementation instead of calling vmcb_is_intercept v2: * svm->nested.save is a separate struct vmcb_save_area_cached, and not vmcb_save_area. * update also vmcb02->cr3 with svm->nested.save.cr3 RFC: * use svm->nested.save instead of local variables. * not dependent anymore from "KVM: nSVM: remove useless kvm_clear_*_queue" * simplified patches, we just use the struct and not move the check nearer to the TOU. Emanuele Giuseppe Esposito (7): KVM: nSVM: move nested_vmcb_check_cr3_cr4 logic in nested_vmcb_valid_sregs nSVM: introduce smv->nested.save to cache save area fields nSVM: rename nested_load_control_from_vmcb12 in nested_copy_vmcb_control_to_cache nSVM: use vmcb_save_area_cached in nested_vmcb_valid_sregs() nSVM: use svm->nested.save to load vmcb12 registers and avoid TOC/TOU races nSVM: introduce struct vmcb_ctrl_area_cached nSVM: use vmcb_ctrl_area_cached instead of vmcb_control_area in struct svm_nested_state arch/x86/kvm/svm/nested.c | 250 ++++++++++++++++++++++++-------------- arch/x86/kvm/svm/svm.c | 7 +- arch/x86/kvm/svm/svm.h | 59 ++++++++- 3 files changed, 213 insertions(+), 103 deletions(-)