linux-kernel - Re: [PATCH v8 08/11] x86/tdx: Wire up KVM hypercalls

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: <YYWbJVTNr5QMTQGb@google.com>
Date:   Fri, 5 Nov 2021 20:59:17 +0000
From:   Sean Christopherson <seanjc@...gle.com>
To:     Kuppuswamy Sathyanarayanan 
        <sathyanarayanan.kuppuswamy@...ux.intel.com>
Cc:     Thomas Gleixner <tglx@...utronix.de>,
        Ingo Molnar <mingo@...hat.com>, Borislav Petkov <bp@...en8.de>,
        x86@...nel.org, Paolo Bonzini <pbonzini@...hat.com>,
        David Hildenbrand <david@...hat.com>,
        Andrea Arcangeli <aarcange@...hat.com>,
        Josh Poimboeuf <jpoimboe@...hat.com>,
        Juergen Gross <jgross@...e.com>, Deep Shah <sdeep@...are.com>,
        VMware Inc <pv-drivers@...are.com>,
        Vitaly Kuznetsov <vkuznets@...hat.com>,
        Wanpeng Li <wanpengli@...cent.com>,
        Jim Mattson <jmattson@...gle.com>,
        Joerg Roedel <joro@...tes.org>, Peter H Anvin <hpa@...or.com>,
        Dave Hansen <dave.hansen@...el.com>,
        Tony Luck <tony.luck@...el.com>,
        Dan Williams <dan.j.williams@...el.com>,
        Andi Kleen <ak@...ux.intel.com>,
        Kirill Shutemov <kirill.shutemov@...ux.intel.com>,
        Kuppuswamy Sathyanarayanan <knsathya@...nel.org>,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v8 08/11] x86/tdx: Wire up KVM hypercalls

On Mon, Oct 04, 2021, Kuppuswamy Sathyanarayanan wrote:
> [Isaku Yamahata: proposed KVM VENDOR string]

...

> diff --git a/arch/x86/include/asm/tdx.h b/arch/x86/include/asm/tdx.h
> index 458a564dd4c2..ebb97e082376 100644
> --- a/arch/x86/include/asm/tdx.h
> +++ b/arch/x86/include/asm/tdx.h
> @@ -6,8 +6,9 @@
>  #include <linux/cpufeature.h>
>  #include <linux/types.h>
>  
> -#define TDX_CPUID_LEAF_ID	0x21
> -#define TDX_HYPERCALL_STANDARD  0
> +#define TDX_CPUID_LEAF_ID			0x21
> +#define TDX_HYPERCALL_STANDARD			0
> +#define TDX_HYPERCALL_VENDOR_KVM		0x4d564b2e584454 /* TDX.KVM */

...

> +#if defined(CONFIG_KVM_GUEST) && defined(CONFIG_INTEL_TDX_GUEST)
> +static inline long tdx_kvm_hypercall(unsigned int nr, unsigned long p1,
> +				     unsigned long p2, unsigned long p3,
> +				     unsigned long p4)
> +{
> +	struct tdx_hypercall_output out;
> +	u64 err;
> +
> +	err = __tdx_hypercall(TDX_HYPERCALL_VENDOR_KVM, nr, p1, p2,
> +			      p3, p4, &out);

Why use a magic string?  There are already mechanisms for the host to announce
itself to the guest, i.e. the guest shouldn't be attempting these hypercalls unless
it knows it's running on KVM (or something that implements KVM's ABI, whatever
that may be).

The only use case I can think of is to support multiple flavors of hypercalls in
the VMM, e.g. to let KVM support both KVM and Hyper-V hypercalls when KVM is
masquerading as Hyper-V, but this magic value alone isn't sufficient.

And if there is a future where KVM wants to support multiple sets of hypercalls,
using the entire 64-bit GPR for a magic value is unnecessary and wasteful, e.g.
it requires an overside MOV imm, reg.

Why not use a single high bit?  Actually, looking at KVM's set of hypercalls,
the guest can literally pass @nr as is.  The GHCI defines all non-zero values as
vendor owned, i.e. this needs to ensure only that @nr is non-zero.  For whatever
reason, perhaps to avoid false positives if the guest forgets to fill the value,
KVM's hypercalls start at '1'.

Regardless of what is stuffed into r10 for the TDVMCALL, if it's anything other
than KVM's raw hypercall number, it absolutely must go into
include/uapi/linux/kvm_para.h and it should be done as a standalone commit,
because what you're proposing here is effectively committing KVM to supporting
an ABI.  That is not remotely clear from the changelog.