Messages in this thread | | | Subject | Re: [RFC PATCH v2 24/69] KVM: x86: Introduce "protected guest" concept and block disallowed ioctls | From | Paolo Bonzini <> | Date | Tue, 6 Jul 2021 15:59:48 +0200 |
| |
On 03/07/21 00:04, isaku.yamahata@intel.com wrote: > From: Sean Christopherson <sean.j.christopherson@intel.com> > > Add 'guest_state_protected' to mark a VM's state as being protected by > hardware/firmware, e.g. SEV-ES or TDX-SEAM. Use the flag to disallow > ioctls() and/or flows that attempt to access protected state. > > Return an error if userspace attempts to get/set register state for a > protected VM, e.g. a non-debug TDX guest. KVM can't provide sane data, > it's userspace's responsibility to avoid attempting to read guest state > when it's known to be inaccessible. > > Retrieving vCPU events is the one exception, as the userspace VMM is > allowed to inject NMIs. > > Co-developed-by: Xiaoyao Li <xiaoyao.li@intel.com> > Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com> > Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com> > Signed-off-by: Isaku Yamahata <isaku.yamahata@intel.com> > --- > arch/x86/kvm/x86.c | 104 +++++++++++++++++++++++++++++++++++++-------- > 1 file changed, 86 insertions(+), 18 deletions(-)
Looks good, but it should be checked whether it breaks QEMU for SEV-ES. Tom, can you help?
Paolo
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c > index 271245ffc67c..b89845dfb679 100644 > --- a/arch/x86/kvm/x86.c > +++ b/arch/x86/kvm/x86.c > @@ -4297,6 +4297,10 @@ static int kvm_vcpu_ioctl_nmi(struct kvm_vcpu *vcpu) > > static int kvm_vcpu_ioctl_smi(struct kvm_vcpu *vcpu) > { > + /* TODO: use more precise flag */ > + if (vcpu->arch.guest_state_protected) > + return -EINVAL; > + > kvm_make_request(KVM_REQ_SMI, vcpu); > > return 0; > @@ -4343,6 +4347,10 @@ static int kvm_vcpu_ioctl_x86_set_mce(struct kvm_vcpu *vcpu, > unsigned bank_num = mcg_cap & 0xff; > u64 *banks = vcpu->arch.mce_banks; > > + /* TODO: use more precise flag */ > + if (vcpu->arch.guest_state_protected) > + return -EINVAL; > + > if (mce->bank >= bank_num || !(mce->status & MCI_STATUS_VAL)) > return -EINVAL; > /* > @@ -4438,7 +4446,8 @@ static void kvm_vcpu_ioctl_x86_get_vcpu_events(struct kvm_vcpu *vcpu, > vcpu->arch.interrupt.injected && !vcpu->arch.interrupt.soft; > events->interrupt.nr = vcpu->arch.interrupt.nr; > events->interrupt.soft = 0; > - events->interrupt.shadow = static_call(kvm_x86_get_interrupt_shadow)(vcpu); > + if (!vcpu->arch.guest_state_protected) > + events->interrupt.shadow = static_call(kvm_x86_get_interrupt_shadow)(vcpu); > > events->nmi.injected = vcpu->arch.nmi_injected; > events->nmi.pending = vcpu->arch.nmi_pending != 0; > @@ -4467,11 +4476,17 @@ static void kvm_smm_changed(struct kvm_vcpu *vcpu); > static int kvm_vcpu_ioctl_x86_set_vcpu_events(struct kvm_vcpu *vcpu, > struct kvm_vcpu_events *events) > { > - if (events->flags & ~(KVM_VCPUEVENT_VALID_NMI_PENDING > - | KVM_VCPUEVENT_VALID_SIPI_VECTOR > - | KVM_VCPUEVENT_VALID_SHADOW > - | KVM_VCPUEVENT_VALID_SMM > - | KVM_VCPUEVENT_VALID_PAYLOAD)) > + u32 allowed_flags = KVM_VCPUEVENT_VALID_NMI_PENDING | > + KVM_VCPUEVENT_VALID_SIPI_VECTOR | > + KVM_VCPUEVENT_VALID_SHADOW | > + KVM_VCPUEVENT_VALID_SMM | > + KVM_VCPUEVENT_VALID_PAYLOAD; > + > + /* TODO: introduce more precise flag */ > + if (vcpu->arch.guest_state_protected) > + allowed_flags = KVM_VCPUEVENT_VALID_NMI_PENDING; > + > + if (events->flags & ~allowed_flags) > return -EINVAL; > > if (events->flags & KVM_VCPUEVENT_VALID_PAYLOAD) { > @@ -4552,17 +4567,22 @@ static int kvm_vcpu_ioctl_x86_set_vcpu_events(struct kvm_vcpu *vcpu, > return 0; > } > > -static void kvm_vcpu_ioctl_x86_get_debugregs(struct kvm_vcpu *vcpu, > - struct kvm_debugregs *dbgregs) > +static int kvm_vcpu_ioctl_x86_get_debugregs(struct kvm_vcpu *vcpu, > + struct kvm_debugregs *dbgregs) > { > unsigned long val; > > + if (vcpu->arch.guest_state_protected) > + return -EINVAL; > + > memcpy(dbgregs->db, vcpu->arch.db, sizeof(vcpu->arch.db)); > kvm_get_dr(vcpu, 6, &val); > dbgregs->dr6 = val; > dbgregs->dr7 = vcpu->arch.dr7; > dbgregs->flags = 0; > memset(&dbgregs->reserved, 0, sizeof(dbgregs->reserved)); > + > + return 0; > } > > static int kvm_vcpu_ioctl_x86_set_debugregs(struct kvm_vcpu *vcpu, > @@ -4576,6 +4596,9 @@ static int kvm_vcpu_ioctl_x86_set_debugregs(struct kvm_vcpu *vcpu, > if (!kvm_dr7_valid(dbgregs->dr7)) > return -EINVAL; > > + if (vcpu->arch.guest_state_protected) > + return -EINVAL; > + > memcpy(vcpu->arch.db, dbgregs->db, sizeof(vcpu->arch.db)); > kvm_update_dr0123(vcpu); > vcpu->arch.dr6 = dbgregs->dr6; > @@ -4671,11 +4694,14 @@ static void load_xsave(struct kvm_vcpu *vcpu, u8 *src) > } > } > > -static void kvm_vcpu_ioctl_x86_get_xsave(struct kvm_vcpu *vcpu, > - struct kvm_xsave *guest_xsave) > +static int kvm_vcpu_ioctl_x86_get_xsave(struct kvm_vcpu *vcpu, > + struct kvm_xsave *guest_xsave) > { > + if (vcpu->arch.guest_state_protected) > + return -EINVAL; > + > if (!vcpu->arch.guest_fpu) > - return; > + return 0; > > if (boot_cpu_has(X86_FEATURE_XSAVE)) { > memset(guest_xsave, 0, sizeof(struct kvm_xsave)); > @@ -4687,6 +4713,8 @@ static void kvm_vcpu_ioctl_x86_get_xsave(struct kvm_vcpu *vcpu, > *(u64 *)&guest_xsave->region[XSAVE_HDR_OFFSET / sizeof(u32)] = > XFEATURE_MASK_FPSSE; > } > + > + return 0; > } > > #define XSAVE_MXCSR_OFFSET 24 > @@ -4697,6 +4725,9 @@ static int kvm_vcpu_ioctl_x86_set_xsave(struct kvm_vcpu *vcpu, > u64 xstate_bv; > u32 mxcsr; > > + if (vcpu->arch.guest_state_protected) > + return -EINVAL; > + > if (!vcpu->arch.guest_fpu) > return 0; > > @@ -4722,18 +4753,22 @@ static int kvm_vcpu_ioctl_x86_set_xsave(struct kvm_vcpu *vcpu, > return 0; > } > > -static void kvm_vcpu_ioctl_x86_get_xcrs(struct kvm_vcpu *vcpu, > - struct kvm_xcrs *guest_xcrs) > +static int kvm_vcpu_ioctl_x86_get_xcrs(struct kvm_vcpu *vcpu, > + struct kvm_xcrs *guest_xcrs) > { > + if (vcpu->arch.guest_state_protected) > + return -EINVAL; > + > if (!boot_cpu_has(X86_FEATURE_XSAVE)) { > guest_xcrs->nr_xcrs = 0; > - return; > + return 0; > } > > guest_xcrs->nr_xcrs = 1; > guest_xcrs->flags = 0; > guest_xcrs->xcrs[0].xcr = XCR_XFEATURE_ENABLED_MASK; > guest_xcrs->xcrs[0].value = vcpu->arch.xcr0; > + return 0; > } > > static int kvm_vcpu_ioctl_x86_set_xcrs(struct kvm_vcpu *vcpu, > @@ -4741,6 +4776,9 @@ static int kvm_vcpu_ioctl_x86_set_xcrs(struct kvm_vcpu *vcpu, > { > int i, r = 0; > > + if (vcpu->arch.guest_state_protected) > + return -EINVAL; > + > if (!boot_cpu_has(X86_FEATURE_XSAVE)) > return -EINVAL; > > @@ -5011,7 +5049,9 @@ long kvm_arch_vcpu_ioctl(struct file *filp, > case KVM_GET_DEBUGREGS: { > struct kvm_debugregs dbgregs; > > - kvm_vcpu_ioctl_x86_get_debugregs(vcpu, &dbgregs); > + r = kvm_vcpu_ioctl_x86_get_debugregs(vcpu, &dbgregs); > + if (r) > + break; > > r = -EFAULT; > if (copy_to_user(argp, &dbgregs, > @@ -5037,7 +5077,9 @@ long kvm_arch_vcpu_ioctl(struct file *filp, > if (!u.xsave) > break; > > - kvm_vcpu_ioctl_x86_get_xsave(vcpu, u.xsave); > + r = kvm_vcpu_ioctl_x86_get_xsave(vcpu, u.xsave); > + if (r) > + break; > > r = -EFAULT; > if (copy_to_user(argp, u.xsave, sizeof(struct kvm_xsave))) > @@ -5061,7 +5103,9 @@ long kvm_arch_vcpu_ioctl(struct file *filp, > if (!u.xcrs) > break; > > - kvm_vcpu_ioctl_x86_get_xcrs(vcpu, u.xcrs); > + r = kvm_vcpu_ioctl_x86_get_xcrs(vcpu, u.xcrs); > + if (r) > + break; > > r = -EFAULT; > if (copy_to_user(argp, u.xcrs, > @@ -9735,6 +9779,12 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu) > goto out; > } > > + if (vcpu->arch.guest_state_protected && > + (kvm_run->kvm_valid_regs || kvm_run->kvm_dirty_regs)) { > + r = -EINVAL; > + goto out; > + } > + > if (kvm_run->kvm_dirty_regs) { > r = sync_regs(vcpu); > if (r != 0) > @@ -9765,7 +9815,7 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu) > > out: > kvm_put_guest_fpu(vcpu); > - if (kvm_run->kvm_valid_regs) > + if (kvm_run->kvm_valid_regs && !vcpu->arch.guest_state_protected) > store_regs(vcpu); > post_kvm_run_save(vcpu); > kvm_sigset_deactivate(vcpu); > @@ -9812,6 +9862,9 @@ static void __get_regs(struct kvm_vcpu *vcpu, struct kvm_regs *regs) > > int kvm_arch_vcpu_ioctl_get_regs(struct kvm_vcpu *vcpu, struct kvm_regs *regs) > { > + if (vcpu->arch.guest_state_protected) > + return -EINVAL; > + > vcpu_load(vcpu); > __get_regs(vcpu, regs); > vcpu_put(vcpu); > @@ -9852,6 +9905,9 @@ static void __set_regs(struct kvm_vcpu *vcpu, struct kvm_regs *regs) > > int kvm_arch_vcpu_ioctl_set_regs(struct kvm_vcpu *vcpu, struct kvm_regs *regs) > { > + if (vcpu->arch.guest_state_protected) > + return -EINVAL; > + > vcpu_load(vcpu); > __set_regs(vcpu, regs); > vcpu_put(vcpu); > @@ -9912,6 +9968,9 @@ static void __get_sregs(struct kvm_vcpu *vcpu, struct kvm_sregs *sregs) > int kvm_arch_vcpu_ioctl_get_sregs(struct kvm_vcpu *vcpu, > struct kvm_sregs *sregs) > { > + if (vcpu->arch.guest_state_protected) > + return -EINVAL; > + > vcpu_load(vcpu); > __get_sregs(vcpu, sregs); > vcpu_put(vcpu); > @@ -10112,6 +10171,9 @@ int kvm_arch_vcpu_ioctl_set_sregs(struct kvm_vcpu *vcpu, > { > int ret; > > + if (vcpu->arch.guest_state_protected) > + return -EINVAL; > + > vcpu_load(vcpu); > ret = __set_sregs(vcpu, sregs); > vcpu_put(vcpu); > @@ -10205,6 +10267,9 @@ int kvm_arch_vcpu_ioctl_get_fpu(struct kvm_vcpu *vcpu, struct kvm_fpu *fpu) > { > struct fxregs_state *fxsave; > > + if (vcpu->arch.guest_state_protected) > + return -EINVAL; > + > if (!vcpu->arch.guest_fpu) > return 0; > > @@ -10228,6 +10293,9 @@ int kvm_arch_vcpu_ioctl_set_fpu(struct kvm_vcpu *vcpu, struct kvm_fpu *fpu) > { > struct fxregs_state *fxsave; > > + if (vcpu->arch.guest_state_protected) > + return -EINVAL; > + > if (!vcpu->arch.guest_fpu) > return 0; > >
| |