| 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: <1ab2699d-eccb-5848-33cb-f8bc87ac4b62@amd.com>
Date: Mon, 17 May 2021 11:35:26 -0500
From: Tom Lendacky <thomas.lendacky@....com>
To: Jon Kohler <jon@...anix.com>, Andy Lutomirski <luto@...nel.org>
Cc: Babu Moger <babu.moger@....com>,
Thomas Gleixner <tglx@...utronix.de>,
Ingo Molnar <mingo@...hat.com>, Borislav Petkov <bp@...en8.de>,
X86 ML <x86@...nel.org>, "H. Peter Anvin" <hpa@...or.com>,
Paolo Bonzini <pbonzini@...hat.com>,
Sean Christopherson <seanjc@...gle.com>,
Vitaly Kuznetsov <vkuznets@...hat.com>,
Wanpeng Li <wanpengli@...cent.com>,
Jim Mattson <jmattson@...gle.com>,
Joerg Roedel <joro@...tes.org>,
Dave Hansen <dave.hansen@...el.com>,
Fenghua Yu <fenghua.yu@...el.com>,
Yu-cheng Yu <yu-cheng.yu@...el.com>,
Tony Luck <tony.luck@...el.com>,
Uros Bizjak <ubizjak@...il.com>,
Petteri Aimonen <jpa@....mail.kapsi.fi>,
Kan Liang <kan.liang@...ux.intel.com>,
Andrew Morton <akpm@...ux-foundation.org>,
Mike Rapoport <rppt@...nel.org>,
Benjamin Thiel <b.thiel@...teo.de>,
Fan Yang <Fan_Yang@...u.edu.cn>,
Juergen Gross <jgross@...e.com>,
Dave Jiang <dave.jiang@...el.com>,
"Peter Zijlstra (Intel)" <peterz@...radead.org>,
Ricardo Neri <ricardo.neri-calderon@...ux.intel.com>,
Arvind Sankar <nivedita@...m.mit.edu>,
LKML <linux-kernel@...r.kernel.org>,
kvm list <kvm@...r.kernel.org>
Subject: Re: [PATCH] KVM: x86: add hint to skip hidden rdpkru under
kvm_load_host_xsave_state
On 5/16/21 9:50 PM, Jon Kohler wrote:
>
>
>> On May 14, 2021, at 1:11 AM, Andy Lutomirski <luto@...nel.org> wrote:
>>
>> On Fri, May 7, 2021 at 9:45 AM Jon Kohler <jon@...anix.com> wrote:
>>>
>>> kvm_load_host_xsave_state handles xsave on vm exit, part of which is
>>> managing memory protection key state. The latest arch.pkru is updated
>>> with a rdpkru, and if that doesn't match the base host_pkru (which
>>> about 70% of the time), we issue a __write_pkru.
>>
>> This thread caused me to read the code, and I don't get how it's even
>> remotely correct.
>>
>> First, kvm_load_guest_fpu() has this delight:
>>
>> /*
>> * Guests with protected state can't have it set by the hypervisor,
>> * so skip trying to set it.
>> */
>> if (vcpu->arch.guest_fpu)
>> /* PKRU is separately restored in kvm_x86_ops.run. */
>> __copy_kernel_to_fpregs(&vcpu->arch.guest_fpu->state,
>> ~XFEATURE_MASK_PKRU);
>>
>> That's nice, but it fails to restore XINUSE[PKRU]. As far as I know,
>> that bit is live, and the only way to restore it to 0 is with
>> XRSTOR(S).
>
> Thanks, Andy - Adding Tom Lendacky to this thread as that
> Particular snippet was last touched in ~5.11 in ed02b213098a.
It sounds like Andy's comment is separate from the changes associated with
commit ed02b213098a, right?
Commit ed02b213098a just added the check for vcpu->arch.guest_fpu to
support SEV-ES guests. Since the hypervisor can't save/restore those
registers directly when running under SEV-ES, we skip this. The state will
be restored when VMRUN is performed.
Thanks,
Tom
>
>>
>>> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
>>> index cebdaa1e3cf5..cd95adbd140c 100644
>>> --- a/arch/x86/kvm/x86.c
>>> +++ b/arch/x86/kvm/x86.c
>>> @@ -912,10 +912,10 @@ void kvm_load_guest_xsave_state(struct kvm_vcpu *vcpu)
>>> }
>>>
>>> if (static_cpu_has(X86_FEATURE_PKU) &&
>>> - (kvm_read_cr4_bits(vcpu, X86_CR4_PKE) ||
>>> - (vcpu->arch.xcr0 & XFEATURE_MASK_PKRU)) &&
>>> - vcpu->arch.pkru != vcpu->arch.host_pkru)
>>> - __write_pkru(vcpu->arch.pkru);
>>> + vcpu->arch.pkru != vcpu->arch.host_pkru &&
>>> + ((vcpu->arch.xcr0 & XFEATURE_MASK_PKRU) ||
>>> + kvm_read_cr4_bits(vcpu, X86_CR4_PKE)))
>>> + __write_pkru(vcpu->arch.pkru, false);
>>
>> Please tell me I'm missing something (e.g. KVM very cleverly managing
>> the PKRU register using intercepts) that makes this reliably load the
>> guest value. An innocent or malicious guest could easily make that
>> condition evaluate to false, thus allowing the host PKRU value to be
>> live in guest mode. (Or is something fancy going on here?)
>>
>> I don't even want to think about what happens if a perf NMI hits and
>> accesses host user memory while the guest PKRU is live (on VMX -- I
>> think this can't happen on SVM).
>
> Perhaps Babu / Dave can comment on the exiting logic here? Babu did
> some additional work to fix save/restore on 37486135d3a.
>
> From this changes perspective, I’m just trying to get to the resultant
> evaluation a bit quicker, which this change (and the v3) seems to do
> ok with; however, if I’ve ran my ship into a larger ice berg, happy to
> collaborate to make it better overall.
>
>>
>>> }
>>> EXPORT_SYMBOL_GPL(kvm_load_guest_xsave_state);
>>>
>>> @@ -925,11 +925,11 @@ void kvm_load_host_xsave_state(struct kvm_vcpu *vcpu)
>>> return;
>>>
>>> if (static_cpu_has(X86_FEATURE_PKU) &&
>>> - (kvm_read_cr4_bits(vcpu, X86_CR4_PKE) ||
>>> - (vcpu->arch.xcr0 & XFEATURE_MASK_PKRU))) {
>>> + ((vcpu->arch.xcr0 & XFEATURE_MASK_PKRU) ||
>>> + kvm_read_cr4_bits(vcpu, X86_CR4_PKE))) {
>>> vcpu->arch.pkru = rdpkru();
>>> if (vcpu->arch.pkru != vcpu->arch.host_pkru)
>>> - __write_pkru(vcpu->arch.host_pkru);
>>> + __write_pkru(vcpu->arch.host_pkru, true);
>>> }
>>
>> Suppose the guest writes to PKRU and then, without exiting, sets PKE =
>> 0 and XCR0[PKRU] = 0. (Or are the intercepts such that this can't
>> happen except on SEV where maybe SEV magic makes the problem go away?)
>>
>> I admit I'm fairly mystified as to why KVM doesn't handle PKRU like
>> the rest of guest XSTATE.
>
> I’ll defer to Dave/Babu here. This code has been this way for a bit now,
> I’m not sure at first glance if that situation could occur intentionally
> or accidentally, but that would evaluate the same both before and
> after this change, no?
>
Powered by blists - more mailing lists