| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <02ca73b2-7f04-813d-5bb7-649c0edafa06@redhat.com>
Date: Tue, 6 Jul 2021 15:59:48 +0200
From: Paolo Bonzini <pbonzini@...hat.com>
To: isaku.yamahata@...el.com, Thomas Gleixner <tglx@...utronix.de>,
Ingo Molnar <mingo@...hat.com>, Borislav Petkov <bp@...en8.de>,
"H . Peter Anvin" <hpa@...or.com>,
Vitaly Kuznetsov <vkuznets@...hat.com>,
Wanpeng Li <wanpengli@...cent.com>,
Jim Mattson <jmattson@...gle.com>,
Joerg Roedel <joro@...tes.org>, erdemaktas@...gle.com,
Connor Kuehl <ckuehl@...hat.com>,
Sean Christopherson <seanjc@...gle.com>, x86@...nel.org,
linux-kernel@...r.kernel.org, kvm@...r.kernel.org,
Tom Lendacky <thomas.lendacky@....com>
Cc: isaku.yamahata@...il.com,
Sean Christopherson <sean.j.christopherson@...el.com>,
Xiaoyao Li <xiaoyao.li@...el.com>
Subject: Re: [RFC PATCH v2 24/69] KVM: x86: Introduce "protected guest"
concept and block disallowed ioctls
On 03/07/21 00:04, isaku.yamahata@...el.com wrote:
> From: Sean Christopherson <sean.j.christopherson@...el.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@...el.com>
> Signed-off-by: Xiaoyao Li <xiaoyao.li@...el.com>
> Signed-off-by: Sean Christopherson <sean.j.christopherson@...el.com>
> Signed-off-by: Isaku Yamahata <isaku.yamahata@...el.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;
>
>
Powered by blists - more mailing lists