linux-kernel - Re: [PATCH v15 13/41] KVM: x86: Enable guest SSP read/write interface with new uAPIs

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <d1bfb652-19ff-434f-bd51-b990543d14d6@intel.com>
Date: Tue, 16 Sep 2025 13:52:36 +0800
From: Xiaoyao Li <xiaoyao.li@...el.com>
To: Sean Christopherson <seanjc@...gle.com>
Cc: Paolo Bonzini <pbonzini@...hat.com>, kvm@...r.kernel.org,
 linux-kernel@...r.kernel.org, Tom Lendacky <thomas.lendacky@....com>,
 Mathias Krause <minipli@...ecurity.net>, John Allen <john.allen@....com>,
 Rick Edgecombe <rick.p.edgecombe@...el.com>, Chao Gao <chao.gao@...el.com>,
 Maxim Levitsky <mlevitsk@...hat.com>, Zhang Yi Z <yi.z.zhang@...ux.intel.com>
Subject: Re: [PATCH v15 13/41] KVM: x86: Enable guest SSP read/write interface
 with new uAPIs

On 9/16/2025 6:12 AM, Sean Christopherson wrote:
> On Mon, Sep 15, 2025, Xiaoyao Li wrote:
>> On 9/13/2025 7:22 AM, Sean Christopherson wrote:
>>> @@ -6097,11 +6105,22 @@ static int kvm_get_set_one_reg(struct kvm_vcpu *vcpu, unsigned int ioctl,
>>>    static int kvm_get_reg_list(struct kvm_vcpu *vcpu,
>>>    			    struct kvm_reg_list __user *user_list)
>>>    {
>>> -	u64 nr_regs = 0;
>>> +	u64 nr_regs = guest_cpu_cap_has(vcpu, X86_FEATURE_SHSTK) ? 1 : 0;
>>
>> I wonder what's the semantic of KVM returning KVM_REG_GUEST_SSP on
>> KVM_GET_REG_LIST. Does it ensure KVM_{G,S}ET_ONE_REG returns -EINVAL on
>> KVM_REG_GUEST_SSP when it's not enumerated by KVM_GET_REG_LIST?
>>
>> If so, but KVM_{G,S}ET_ONE_REG can succeed on GUEST_SSP even if
>> !guest_cpu_cap_has() when @ignore_msrs is true.
> 
> Ugh, great catch.  Too many knobs.  The best idea I've got it to to exempt KVM-
> internal MSRs from ignore_msrs and report_ignored_msrs on host-initiated writes.
> That's unfortunately still a userspace visible change, and would continue to be
> userspace-visible, e.g. if we wanted to change the magic value for
> MSR_KVM_INTERNAL_GUEST_SSP.
> 
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index c78acab2ff3f..6a50261d1c5c 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -511,6 +511,11 @@ static bool kvm_is_advertised_msr(u32 msr_index)
>          return false;
>   }
>   
> +static bool kvm_is_internal_msr(u32 msr)
> +{
> +       return msr == MSR_KVM_INTERNAL_GUEST_SSP;
> +}
> +
>   typedef int (*msr_access_t)(struct kvm_vcpu *vcpu, u32 index, u64 *data,
>                              bool host_initiated);
>   
> @@ -544,6 +549,9 @@ static __always_inline int kvm_do_msr_access(struct kvm_vcpu *vcpu, u32 msr,
>          if (host_initiated && !*data && kvm_is_advertised_msr(msr))
>                  return 0;
>   
> +       if (host_initiated && kvm_is_internal_msr(msr))
> +               return ret;
> +
>          if (!ignore_msrs) {
>                  kvm_debug_ratelimited("unhandled %s: 0x%x data 0x%llx\n",
>                                        op, msr, *data);
> 
> Alternatively, simply exempt host writes from ignore_msrs.  Aha!  And KVM even
> documents that as the behavior:
> 
> 	kvm.ignore_msrs=[KVM] Ignore guest accesses to unhandled MSRs.
> 			Default is 0 (don't ignore, but inject #GP)
> 
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index c78acab2ff3f..177253e75b41 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -544,7 +544,7 @@ static __always_inline int kvm_do_msr_access(struct kvm_vcpu *vcpu, u32 msr,
>          if (host_initiated && !*data && kvm_is_advertised_msr(msr))
>                  return 0;
>   
> -       if (!ignore_msrs) {
> +       if (host_initiated || !ignore_msrs) {
>                  kvm_debug_ratelimited("unhandled %s: 0x%x data 0x%llx\n",
>                                        op, msr, *data);
>                  return ret;
> 
> So while it's technically an ABI change (arguable since it's guarded by an
> off-by-default param), I suspect we can get away with it.  Hmm, commit 6abe9c1386e5
> ("KVM: X86: Move ignore_msrs handling upper the stack") exempted KVM-internal
> MSR accesses from ignore_msrs, but doesn't provide much in the way of justification
> for _why_ that's desirable.
> 
> Argh, and that same mini-series extended the behavior to feature MSRs, again
> without seeming to consider whether or not it's actually desirable to suppress
> bad VMM accesses.  Even worse, that decision likely generated an absurd amount
> of churn and noise due to splattering helpers and variants all over the place. :-(
> 
> commit 12bc2132b15e0a969b3f455d90a5f215ef239eff
> Author:     Peter Xu <peterx@...hat.com>
> AuthorDate: Mon Jun 22 18:04:42 2020 -0400
> Commit:     Paolo Bonzini <pbonzini@...hat.com>
> CommitDate: Wed Jul 8 16:21:40 2020 -0400
> 
>      KVM: X86: Do the same ignore_msrs check for feature msrs
>      
>      Logically the ignore_msrs and report_ignored_msrs should also apply to feature
>      MSRs.  Add them in.
> 
> For 6.18, I think the safe play is to go with the first path (exempt KVM-internal
> MSRs), and then try to go for the second approach (exempt all host accesses) for
> 6.19.  KVM's ABI for ignore_msrs=true is already all kinds of messed up, so I'm
> not terribly concerned about temporarily making it marginally worse.

Looks OK to me.