| Message ID | 20210903102039.55422-4-eesposit@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: nSVM: avoid TOC/TOU race when checking vmcb12 | expand |
On Fri, 2021-09-03 at 12:20 +0200, Emanuele Giuseppe Esposito wrote: > Move the checks done by nested_vmcb_valid_sregs and > nested_vmcb_check_controls directly in enter_svm_guest_mode, > and use svm->nested.save cached fields (EFER, CR0, CR4) > instead of vmcb12's. > This prevents from creating TOC/TOU races. > > This also avoids the need of force-setting EFER_SVME in > nested_vmcb02_prepare_save. > > Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com> > --- > arch/x86/kvm/svm/nested.c | 23 ++++++----------------- > 1 file changed, 6 insertions(+), 17 deletions(-) > > diff --git a/arch/x86/kvm/svm/nested.c b/arch/x86/kvm/svm/nested.c > index 2491c77203c7..487810cfefde 100644 > --- a/arch/x86/kvm/svm/nested.c > +++ b/arch/x86/kvm/svm/nested.c > @@ -280,13 +280,6 @@ static bool nested_vmcb_check_controls(struct kvm_vcpu *vcpu, > static bool nested_vmcb_valid_sregs(struct kvm_vcpu *vcpu, > struct vmcb_save_area *save) > { > - /* > - * FIXME: these should be done after copying the fields, > - * to avoid TOC/TOU races. For these save area checks > - * the possible damage is limited since kvm_set_cr0 and > - * kvm_set_cr4 handle failure; EFER_SVME is an exception > - * so it is force-set later in nested_prepare_vmcb_save. > - */ > if (CC(!(save->efer & EFER_SVME))) > return false; > > @@ -459,7 +452,8 @@ void nested_vmcb02_compute_g_pat(struct vcpu_svm *svm) > svm->nested.vmcb02.ptr->save.g_pat = svm->vmcb01.ptr->save.g_pat; > } > > -static void nested_vmcb02_prepare_save(struct vcpu_svm *svm, struct vmcb *vmcb12) > +static void nested_vmcb02_prepare_save(struct vcpu_svm *svm, > + struct vmcb *vmcb12) Tiny nitpick: the kernel these days allow up to 100 characters in a line, thus this change is not needed IMHO. > { > bool new_vmcb12 = false; > > @@ -488,15 +482,10 @@ static void nested_vmcb02_prepare_save(struct vcpu_svm *svm, struct vmcb *vmcb12 > > kvm_set_rflags(&svm->vcpu, vmcb12->save.rflags | X86_EFLAGS_FIXED); > > - /* > - * Force-set EFER_SVME even though it is checked earlier on the > - * VMCB12, because the guest can flip the bit between the check > - * and now. Clearing EFER_SVME would call svm_free_nested. > - */ > - svm_set_efer(&svm->vcpu, vmcb12->save.efer | EFER_SVME); > + svm_set_efer(&svm->vcpu, svm->nested.save.efer); > > - svm_set_cr0(&svm->vcpu, vmcb12->save.cr0); > - svm_set_cr4(&svm->vcpu, vmcb12->save.cr4); > + svm_set_cr0(&svm->vcpu, svm->nested.save.cr0); > + svm_set_cr4(&svm->vcpu, svm->nested.save.cr4); > > svm->vcpu.arch.cr2 = vmcb12->save.cr2; > > @@ -671,7 +660,7 @@ int nested_svm_vmrun(struct kvm_vcpu *vcpu) > nested_load_control_from_vmcb12(svm, &vmcb12->control); > nested_load_save_from_vmcb12(svm, &vmcb12->save); > > - if (!nested_vmcb_valid_sregs(vcpu, &vmcb12->save) || > + if (!nested_vmcb_valid_sregs(vcpu, &svm->nested.save) || > !nested_vmcb_check_controls(vcpu, &svm->nested.ctl)) { If you use a different struct for the copied fields, then it makes sense IMHO to drop the 'control' parameter from nested_vmcb_check_controls, and just use the svm->nested.save there directly. > vmcb12->control.exit_code = SVM_EXIT_ERR; > vmcb12->control.exit_code_hi = 0; I think you forgot to use svm->nested.save.cr3. It is used in enter_svm_guest_mode to setup the mmu. While there are likely no TOC/TOU races in regard to it, it is still better to be consistent about it. Looks very good otherwise. Best regards, Maxim Levitsky
On 12/09/2021 12:42, Maxim Levitsky wrote: >> >> - if (!nested_vmcb_valid_sregs(vcpu, &vmcb12->save) || >> + if (!nested_vmcb_valid_sregs(vcpu, &svm->nested.save) || >> !nested_vmcb_check_controls(vcpu, &svm->nested.ctl)) { > If you use a different struct for the copied fields, then it makes > sense IMHO to drop the 'control' parameter from nested_vmcb_check_controls, > and just use the svm->nested.save there directly. > Ok, what you say in patch 2 makes sense to me. I can create a new struct vmcb_save_area_cached, but I need to keep nested.ctl because 1) it is used also elsewhere, and different fields from the one checked here are read/set and 2) using another structure (or the same vmcb_save_area_cached) in its place would just duplicate the same fields of nested.ctl, creating even more confusion and possible inconsistency. Let me know if you disagree. Thank you, Emanuele
On Tue, 2021-09-14 at 10:20 +0200, Emanuele Giuseppe Esposito wrote: > > On 12/09/2021 12:42, Maxim Levitsky wrote: > > > > > > - if (!nested_vmcb_valid_sregs(vcpu, &vmcb12->save) || > > > + if (!nested_vmcb_valid_sregs(vcpu, &svm->nested.save) || > > > !nested_vmcb_check_controls(vcpu, &svm->nested.ctl)) { > > If you use a different struct for the copied fields, then it makes > > sense IMHO to drop the 'control' parameter from nested_vmcb_check_controls, > > and just use the svm->nested.save there directly. > > > > Ok, what you say in patch 2 makes sense to me. I can create a new struct > vmcb_save_area_cached, but I need to keep nested.ctl because 1) it is > used also elsewhere, and different fields from the one checked here are > read/set and 2) using another structure (or the same Yes, keep nested.ctl, since vast majority of the fields are copied I think. Best regards, Maxim Levitsky > vmcb_save_area_cached) in its place would just duplicate the same fields > of nested.ctl, creating even more confusion and possible inconsistency. > > Let me know if you disagree. > > Thank you, > Emanuele >
On Tue, 2021-09-14 at 12:02 +0300, Maxim Levitsky wrote: > On Tue, 2021-09-14 at 10:20 +0200, Emanuele Giuseppe Esposito wrote: > > On 12/09/2021 12:42, Maxim Levitsky wrote: > > > > > > > > - if (!nested_vmcb_valid_sregs(vcpu, &vmcb12->save) || > > > > + if (!nested_vmcb_valid_sregs(vcpu, &svm->nested.save) || > > > > !nested_vmcb_check_controls(vcpu, &svm->nested.ctl)) { > > > If you use a different struct for the copied fields, then it makes > > > sense IMHO to drop the 'control' parameter from nested_vmcb_check_controls, > > > and just use the svm->nested.save there directly. > > > > > > > Ok, what you say in patch 2 makes sense to me. I can create a new struct > > vmcb_save_area_cached, but I need to keep nested.ctl because 1) it is > > used also elsewhere, and different fields from the one checked here are > > read/set and 2) using another structure (or the same > > Yes, keep nested.ctl, since vast majority of the fields are copied I think. But actually that you mention it, I'll say why not to create vmcb_control_area_cached as well indeed and change the type of svm->nested.save to it. (in a separate patch) I see what you mean that we modify it a bit (but we shoudn't to be honest) and such, but all of this can be fixed. The advantage of having vmcb_control_area_cached is that it becomes impossible to use by mistake a non copied field from the guest. It would also emphasize that this stuff came from the guest and should be treated as a toxic waste. Note again that this should be done if we agree as a separate patch. > > Best regards, > Maxim Levitsky > > > > vmcb_save_area_cached) in its place would just duplicate the same fields > > of nested.ctl, creating even more confusion and possible inconsistency. > > > > Let me know if you disagree. > > > > Thank you, > > Emanuele > >
On 14/09/2021 11:12, Maxim Levitsky wrote: > On Tue, 2021-09-14 at 12:02 +0300, Maxim Levitsky wrote: >> On Tue, 2021-09-14 at 10:20 +0200, Emanuele Giuseppe Esposito wrote: >>> On 12/09/2021 12:42, Maxim Levitsky wrote: >>>>> >>>>> - if (!nested_vmcb_valid_sregs(vcpu, &vmcb12->save) || >>>>> + if (!nested_vmcb_valid_sregs(vcpu, &svm->nested.save) || >>>>> !nested_vmcb_check_controls(vcpu, &svm->nested.ctl)) { >>>> If you use a different struct for the copied fields, then it makes >>>> sense IMHO to drop the 'control' parameter from nested_vmcb_check_controls, >>>> and just use the svm->nested.save there directly. >>>> >>> >>> Ok, what you say in patch 2 makes sense to me. I can create a new struct >>> vmcb_save_area_cached, but I need to keep nested.ctl because 1) it is >>> used also elsewhere, and different fields from the one checked here are >>> read/set and 2) using another structure (or the same >> >> Yes, keep nested.ctl, since vast majority of the fields are copied I think. > > But actually that you mention it, I'll say why not to create vmcb_control_area_cached > as well indeed and change the type of svm->nested.save to it. (in a separate patch) > > I see what you mean that we modify it a bit (but we shoudn't to be honest) and such, but > all of this can be fixed. So basically you are proposing: struct svm_nested_state { ... struct vmcb_control_area ctl; // we need this because it is used everywhere, I think struct vmcb_control_area_cached ctl_cached; struct vmcb_save_area_cached save_cached; ... } and then if (!nested_vmcb_valid_sregs(vcpu, &svm->nested.save_cached) || !nested_vmcb_check_controls(vcpu, &svm->nested.ctl_cached)) { like that? Or do you want to delete nested.ctl completely and just keep the fields actually used in ctl_cached? Also, note that as I am trying to use vmcb_save_area_cached, it is worth noticing that nested_vmcb_valid_sregs() is also used in svm_set_nested_state(), so it requires some additional little changes. Thank you, Emanuele > > The advantage of having vmcb_control_area_cached is that it becomes impossible to use > by mistake a non copied field from the guest. > > It would also emphasize that this stuff came from the guest and should be treated as > a toxic waste. > > Note again that this should be done if we agree as a separate patch. > >> >> Best regards, >> Maxim Levitsky >> >> >>> vmcb_save_area_cached) in its place would just duplicate the same fields >>> of nested.ctl, creating even more confusion and possible inconsistency. >>> >>> Let me know if you disagree. >>> >>> Thank you, >>> Emanuele >>> > >
On Tue, 2021-09-14 at 11:24 +0200, Emanuele Giuseppe Esposito wrote: > > On 14/09/2021 11:12, Maxim Levitsky wrote: > > On Tue, 2021-09-14 at 12:02 +0300, Maxim Levitsky wrote: > > > On Tue, 2021-09-14 at 10:20 +0200, Emanuele Giuseppe Esposito wrote: > > > > On 12/09/2021 12:42, Maxim Levitsky wrote: > > > > > > > > > > > > - if (!nested_vmcb_valid_sregs(vcpu, &vmcb12->save) || > > > > > > + if (!nested_vmcb_valid_sregs(vcpu, &svm->nested.save) || > > > > > > !nested_vmcb_check_controls(vcpu, &svm->nested.ctl)) { > > > > > If you use a different struct for the copied fields, then it makes > > > > > sense IMHO to drop the 'control' parameter from nested_vmcb_check_controls, > > > > > and just use the svm->nested.save there directly. > > > > > > > > > > > > > Ok, what you say in patch 2 makes sense to me. I can create a new struct > > > > vmcb_save_area_cached, but I need to keep nested.ctl because 1) it is > > > > used also elsewhere, and different fields from the one checked here are > > > > read/set and 2) using another structure (or the same > > > > > > Yes, keep nested.ctl, since vast majority of the fields are copied I think. > > > > But actually that you mention it, I'll say why not to create vmcb_control_area_cached > > as well indeed and change the type of svm->nested.save to it. (in a separate patch) > > > > I see what you mean that we modify it a bit (but we shoudn't to be honest) and such, but > > all of this can be fixed. > > So basically you are proposing: > > struct svm_nested_state { > ... > struct vmcb_control_area ctl; // we need this because it is used > everywhere, I think > struct vmcb_control_area_cached ctl_cached; > struct vmcb_save_area_cached save_cached; > ... > } > > and then > > if (!nested_vmcb_valid_sregs(vcpu, &svm->nested.save_cached) || > !nested_vmcb_check_controls(vcpu, &svm->nested.ctl_cached)) { > > like that? > > Or do you want to delete nested.ctl completely and just keep the fields > actually used in ctl_cached? I would do it this way: struct svm_nested_state { ... /* cached fields from the vmcb12 */ struct vmcb_control_area_cached ctl; struct vmcb_save_area_cached save; ... }; Best regards, Maxim Levitsky > > > Also, note that as I am trying to use vmcb_save_area_cached, it is worth > noticing that nested_vmcb_valid_sregs() is also used in > svm_set_nested_state(), so it requires some additional little changes. > > Thank you, > Emanuele > > > The advantage of having vmcb_control_area_cached is that it becomes impossible to use > > by mistake a non copied field from the guest. > > > > It would also emphasize that this stuff came from the guest and should be treated as > > a toxic waste. > > > > Note again that this should be done if we agree as a separate patch. > > > > > Best regards, > > > Maxim Levitsky > > > > > > > > > > vmcb_save_area_cached) in its place would just duplicate the same fields > > > > of nested.ctl, creating even more confusion and possible inconsistency. > > > > > > > > Let me know if you disagree. > > > > > > > > Thank you, > > > > Emanuele > > > >
> > I would do it this way: > > struct svm_nested_state { > ... > /* cached fields from the vmcb12 */ > struct vmcb_control_area_cached ctl; > struct vmcb_save_area_cached save; > ... > }; > > The only thing that requires a little bit of additional work when applying this is svm_get_nested_state() (and theoretically svm_set_nested_state(), in option 2). In this function, nested.ctl is copied in user_vmcb->control. But now nested.ctl is not anymore a vmcb_control_area, so the sizes differ. There are 2 options here: 1) copy nested.ctl into a full vmcb_control_area, and copy it to user space without modifying the API. The advantage is that the API is left intact, but an additional copy is required. 2) modify KVM_GET_NESTED_STATE and KVM_SET_NESTED_STATE to handle vmcb_control_area_cached. Advantage is that there is a lightweight copy + the benefits explained by you in the previous email (no unset field). I am not sure which one is the preferred way here. Thank you, Emanuele
On Tue, 2021-09-14 at 12:52 +0200, Emanuele Giuseppe Esposito wrote: > > I would do it this way: > > > > struct svm_nested_state { > > ... > > /* cached fields from the vmcb12 */ > > struct vmcb_control_area_cached ctl; > > struct vmcb_save_area_cached save; > > ... > > }; > > > > > > The only thing that requires a little bit of additional work when > applying this is svm_get_nested_state() (and theoretically > svm_set_nested_state(), in option 2). In this function, nested.ctl is > copied in user_vmcb->control. But now nested.ctl is not anymore a > vmcb_control_area, so the sizes differ. > > There are 2 options here: > 1) copy nested.ctl into a full vmcb_control_area, and copy it to user > space without modifying the API. The advantage is that the API is left > intact, but an additional copy is required. Thankfully there KVM_GET_NESTED_STATE is not performance critical at all, so a copy isn't that big problem, other that it is a bit ugly. Ugh.. > > 2) modify KVM_GET_NESTED_STATE and KVM_SET_NESTED_STATE to handle > vmcb_control_area_cached. Advantage is that there is a lightweight copy > + the benefits explained by you in the previous email (no unset field). That would break the KVM_GET_NESTED_STATE ABI without a very good reason, especially since some of the currently unused fields in the ctl (there are I think very few of them), might became used later on, needing to break the ABI again. Best regards, Maxim Levitsky > > I am not sure which one is the preferred way here. > > Thank you, > Emanuele >
On 14/09/21 11:12, Maxim Levitsky wrote: > But actually that you mention it, I'll say why not to create vmcb_control_area_cached > as well indeed and change the type of svm->nested.save to it. (in a separate patch) I think you should have two structs, struct vmcb12_control_area_cache and struct vmcb12_save_area_cache. Otherwise the two series that Emanuele posted are nice and can be combined into just one. Paolo
diff --git a/arch/x86/kvm/svm/nested.c b/arch/x86/kvm/svm/nested.c index 2491c77203c7..487810cfefde 100644 --- a/arch/x86/kvm/svm/nested.c +++ b/arch/x86/kvm/svm/nested.c @@ -280,13 +280,6 @@ static bool nested_vmcb_check_controls(struct kvm_vcpu *vcpu, static bool nested_vmcb_valid_sregs(struct kvm_vcpu *vcpu, struct vmcb_save_area *save) { - /* - * FIXME: these should be done after copying the fields, - * to avoid TOC/TOU races. For these save area checks - * the possible damage is limited since kvm_set_cr0 and - * kvm_set_cr4 handle failure; EFER_SVME is an exception - * so it is force-set later in nested_prepare_vmcb_save. - */ if (CC(!(save->efer & EFER_SVME))) return false; @@ -459,7 +452,8 @@ void nested_vmcb02_compute_g_pat(struct vcpu_svm *svm) svm->nested.vmcb02.ptr->save.g_pat = svm->vmcb01.ptr->save.g_pat; } -static void nested_vmcb02_prepare_save(struct vcpu_svm *svm, struct vmcb *vmcb12) +static void nested_vmcb02_prepare_save(struct vcpu_svm *svm, + struct vmcb *vmcb12) { bool new_vmcb12 = false; @@ -488,15 +482,10 @@ static void nested_vmcb02_prepare_save(struct vcpu_svm *svm, struct vmcb *vmcb12 kvm_set_rflags(&svm->vcpu, vmcb12->save.rflags | X86_EFLAGS_FIXED); - /* - * Force-set EFER_SVME even though it is checked earlier on the - * VMCB12, because the guest can flip the bit between the check - * and now. Clearing EFER_SVME would call svm_free_nested. - */ - svm_set_efer(&svm->vcpu, vmcb12->save.efer | EFER_SVME); + svm_set_efer(&svm->vcpu, svm->nested.save.efer); - svm_set_cr0(&svm->vcpu, vmcb12->save.cr0); - svm_set_cr4(&svm->vcpu, vmcb12->save.cr4); + svm_set_cr0(&svm->vcpu, svm->nested.save.cr0); + svm_set_cr4(&svm->vcpu, svm->nested.save.cr4); svm->vcpu.arch.cr2 = vmcb12->save.cr2; @@ -671,7 +660,7 @@ int nested_svm_vmrun(struct kvm_vcpu *vcpu) nested_load_control_from_vmcb12(svm, &vmcb12->control); nested_load_save_from_vmcb12(svm, &vmcb12->save); - if (!nested_vmcb_valid_sregs(vcpu, &vmcb12->save) || + if (!nested_vmcb_valid_sregs(vcpu, &svm->nested.save) || !nested_vmcb_check_controls(vcpu, &svm->nested.ctl)) { vmcb12->control.exit_code = SVM_EXIT_ERR; vmcb12->control.exit_code_hi = 0;
Move the checks done by nested_vmcb_valid_sregs and nested_vmcb_check_controls directly in enter_svm_guest_mode, and use svm->nested.save cached fields (EFER, CR0, CR4) instead of vmcb12's. This prevents from creating TOC/TOU races. This also avoids the need of force-setting EFER_SVME in nested_vmcb02_prepare_save. Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com> --- arch/x86/kvm/svm/nested.c | 23 ++++++----------------- 1 file changed, 6 insertions(+), 17 deletions(-)