linux-kernel - Re: [RFC PATCH v5 089/104] KVM: TDX: Add a placeholder for handler of TDX hypercalls (TDG.VP.VMCALL)

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: <Yk8pX8jweGu745Uj@google.com>
Date:   Thu, 7 Apr 2022 18:11:43 +0000
From:   Sean Christopherson <seanjc@...gle.com>
To:     Paolo Bonzini <pbonzini@...hat.com>
Cc:     Kai Huang <kai.huang@...el.com>, isaku.yamahata@...el.com,
        kvm@...r.kernel.org, linux-kernel@...r.kernel.org,
        isaku.yamahata@...il.com, Jim Mattson <jmattson@...gle.com>,
        erdemaktas@...gle.com, Connor Kuehl <ckuehl@...hat.com>
Subject: Re: [RFC PATCH v5 089/104] KVM: TDX: Add a placeholder for handler
 of TDX hypercalls (TDG.VP.VMCALL)

On Thu, Apr 07, 2022, Paolo Bonzini wrote:
> On 4/7/22 16:39, Sean Christopherson wrote:
> > On Thu, Apr 07, 2022, Paolo Bonzini wrote:
> > > On 4/7/22 06:15, Kai Huang wrote:
> > > > > +static int handle_tdvmcall(struct kvm_vcpu *vcpu)
> > > > > +{
> > > > > +	struct vcpu_tdx *tdx = to_tdx(vcpu);
> > > > > +
> > > > > +	if (unlikely(tdx->tdvmcall.xmm_mask))
> > > > > +		goto unsupported;
> > > > Put a comment explaining this logic?
> > > > 
> > > 
> > > This only seems to be necessary for Hyper-V hypercalls, which however are
> > > not supported by this series in TDX guests (because the kvm_hv_hypercall
> > > still calls kvm_*_read, likewise for Xen).
> > > 
> > > So for now this conditional can be dropped.
> > 
> > I'd prefer to keep the sanity check, it's a cheap and easy way to detect a clear
> > cut guest bug.
> 
> I don't think it's necessarily a guest bug, just silly but valid behavior.

It's a bug from a security perspective given that letting the host unnecessarily
manipulate register state is an exploit waiting to happen.

Though for KVM to reject the TDVMCALLs, the GHCI should really be updated to state
that exposing more state than is required _may_ be considered invalid ("may" so
that KVM isn't required to check the mask on every exit, which IMO is beyond tedious).