| 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: <209a57e9-ca9c-3939-4aaa-4602e3dd7cdd@amd.com>
Date: Tue, 20 Jul 2021 17:08:24 -0500
From: Tom Lendacky <thomas.lendacky@....com>
To: Paolo Bonzini <pbonzini@...hat.com>, 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
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 7/6/21 8:59 AM, Paolo Bonzini wrote:
> 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?
Sorry to take so long to get back to you... been really slammed, let me
look into this a bit more. But, some quick thoughts...
Offhand, the SMI isn't a problem since SEV-ES doesn't support SMM.
For kvm_vcpu_ioctl_x86_{get,set}_xsave(), can TDX use what was added for
SEV-ES:
ed02b213098a ("KVM: SVM: Guest FPU state save/restore not needed for SEV-ES guest")
Same for kvm_arch_vcpu_ioctl_{get,set}_fpu().
The changes to kvm_arch_vcpu_ioctl_{get,set}_sregs() might cause issues,
since there are specific things allowed in __{get,set}_sregs. But I'll
need to dig a bit more on that.
Thanks,
Tom
>
> 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