| 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: <20230227212016.GD4175971@ls.amr.corp.intel.com>
Date: Mon, 27 Feb 2023 13:20:16 -0800
From: Isaku Yamahata <isaku.yamahata@...il.com>
To: "Huang, Kai" <kai.huang@...el.com>
Cc: "kvm@...r.kernel.org" <kvm@...r.kernel.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"Yamahata, Isaku" <isaku.yamahata@...el.com>,
"pbonzini@...hat.com" <pbonzini@...hat.com>,
"Shahar, Sagi" <sagis@...gle.com>,
"Aktas, Erdem" <erdemaktas@...gle.com>,
"isaku.yamahata@...il.com" <isaku.yamahata@...il.com>,
"dmatlack@...gle.com" <dmatlack@...gle.com>,
"Christopherson,, Sean" <seanjc@...gle.com>
Subject: Re: [PATCH v11 014/113] x86/virt/tdx: Add a helper function to
return system wide info about TDX module
On Mon, Jan 16, 2023 at 04:19:47AM +0000,
"Huang, Kai" <kai.huang@...el.com> wrote:
> On Thu, 2023-01-12 at 08:31 -0800, isaku.yamahata@...el.com wrote:
> > From: Isaku Yamahata <isaku.yamahata@...el.com>
> >
> > TDX KVM needs system-wide information about the TDX module, struct
> > tdsysinfo_struct. Add a helper function tdx_get_sysinfo() to return it
> > instead of KVM getting it with various error checks. Make KVM call the
> > function and stash the info. Move out the struct definition about it to
> > common place arch/x86/include/asm/tdx.h.
> >
> > Signed-off-by: Isaku Yamahata <isaku.yamahata@...el.com>
> > ---
> > arch/x86/include/asm/tdx.h | 54 +++++++++++++++++++++++++++++++++++++
> > arch/x86/kvm/vmx/tdx.c | 49 ++++++++++++++++++++++++++++++++-
> > arch/x86/virt/vmx/tdx/tdx.c | 21 ++++++++++++---
> > arch/x86/virt/vmx/tdx/tdx.h | 51 -----------------------------------
> > 4 files changed, 119 insertions(+), 56 deletions(-)
> >
> > diff --git a/arch/x86/include/asm/tdx.h b/arch/x86/include/asm/tdx.h
> > index ed9cf61ff8b4..2ca6e8ce1e43 100644
> > --- a/arch/x86/include/asm/tdx.h
> > +++ b/arch/x86/include/asm/tdx.h
> > @@ -105,6 +105,58 @@ static inline long tdx_kvm_hypercall(unsigned int nr, unsigned long p1,
> > #endif /* CONFIG_INTEL_TDX_GUEST && CONFIG_KVM_GUEST */
> >
> > #ifdef CONFIG_INTEL_TDX_HOST
> > +struct tdx_cpuid_config {
> > + u32 leaf;
> > + u32 sub_leaf;
> > + u32 eax;
> > + u32 ebx;
> > + u32 ecx;
> > + u32 edx;
> > +} __packed;
> > +
> > +#define TDSYSINFO_STRUCT_SIZE 1024
> > +#define TDSYSINFO_STRUCT_ALIGNMENT 1024
> > +
> > +/*
> > + * The size of this structure itself is flexible. The actual structure
> > + * passed to TDH.SYS.INFO must be padded to TDSYSINFO_STRUCT_SIZE and be
> > + * aligned to TDSYSINFO_STRUCT_ALIGNMENT using DECLARE_PADDED_STRUCT().
> > + */
> > +struct tdsysinfo_struct {
> > + /* TDX-SEAM Module Info */
> > + u32 attributes;
> > + u32 vendor_id;
> > + u32 build_date;
> > + u16 build_num;
> > + u16 minor_version;
> > + u16 major_version;
> > + u8 reserved0[14];
> > + /* Memory Info */
> > + u16 max_tdmrs;
> > + u16 max_reserved_per_tdmr;
> > + u16 pamt_entry_size;
> > + u8 reserved1[10];
> > + /* Control Struct Info */
> > + u16 tdcs_base_size;
> > + u8 reserved2[2];
> > + u16 tdvps_base_size;
> > + u8 tdvps_xfam_dependent_size;
> > + u8 reserved3[9];
> > + /* TD Capabilities */
> > + u64 attributes_fixed0;
> > + u64 attributes_fixed1;
> > + u64 xfam_fixed0;
> > + u64 xfam_fixed1;
> > + u8 reserved4[32];
> > + u32 num_cpuid_config;
> > + /*
> > + * The actual number of CPUID_CONFIG depends on above
> > + * 'num_cpuid_config'.
> > + */
> > + DECLARE_FLEX_ARRAY(struct tdx_cpuid_config, cpuid_configs);
> > +} __packed;
> > +
> > +const struct tdsysinfo_struct *tdx_get_sysinfo(void);
> > bool platform_tdx_enabled(void);
> > int tdx_enable(void);
> > /*
> > @@ -120,6 +172,8 @@ void tdx_keyid_free(int keyid);
> > u64 __seamcall(u64 op, u64 rcx, u64 rdx, u64 r8, u64 r9,
> > struct tdx_module_output *out);
> > #else /* !CONFIG_INTEL_TDX_HOST */
> > +struct tdsysinfo_struct;
> > +static inline const struct tdsysinfo_struct *tdx_get_sysinfo(void) { return NULL; }
> > static inline bool platform_tdx_enabled(void) { return false; }
> > static inline int tdx_enable(void) { return -EINVAL; }
> > static inline int tdx_keyid_alloc(void) { return -EOPNOTSUPP; }
> > diff --git a/arch/x86/kvm/vmx/tdx.c b/arch/x86/kvm/vmx/tdx.c
> > index 6c7d9ec53046..2adf5551ab26 100644
> > --- a/arch/x86/kvm/vmx/tdx.c
> > +++ b/arch/x86/kvm/vmx/tdx.c
> > @@ -11,9 +11,34 @@
> > #undef pr_fmt
> > #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> >
> > +#define TDX_MAX_NR_CPUID_CONFIGS \
> > + ((TDSYSINFO_STRUCT_SIZE - \
> > + offsetof(struct tdsysinfo_struct, cpuid_configs)) \
> > + / sizeof(struct tdx_cpuid_config))
> > +
> > +struct tdx_capabilities {
> > + u8 tdcs_nr_pages;
> > + u8 tdvpx_nr_pages;
> > +
> > + u64 attrs_fixed0;
> > + u64 attrs_fixed1;
> > + u64 xfam_fixed0;
> > + u64 xfam_fixed1;
> > +
> > + u32 nr_cpuid_configs;
> > + struct tdx_cpuid_config cpuid_configs[TDX_MAX_NR_CPUID_CONFIGS];
> > +};
> > +
> > +/* Capabilities of KVM + the TDX module. */
> > +static struct tdx_capabilities tdx_caps;
> > +
> > static int __init tdx_module_setup(void)
> > {
> > - int ret;
> > + const struct tdsysinfo_struct *tdsysinfo;
> > + int ret = 0;
> > +
> > + BUILD_BUG_ON(sizeof(*tdsysinfo) > TDSYSINFO_STRUCT_SIZE);
> > + BUILD_BUG_ON(TDX_MAX_NR_CPUID_CONFIGS != 37);
> >
> > ret = tdx_enable();
> > if (ret) {
> > @@ -21,6 +46,28 @@ static int __init tdx_module_setup(void)
> > return ret;
> > }
> >
> > + tdsysinfo = tdx_get_sysinfo();
> > + if (tdsysinfo->num_cpuid_config > TDX_MAX_NR_CPUID_CONFIGS)
> > + return -EIO;
>
> This check basically means TDX module is buggy (or kernel has bug). Do we
> really need this check? Is TDX module that buggy?
>
> IMHO you don't need this one, or use WARN() if you want to catch kernel bug?
Ok, I made it WARN_ON().
> > +
> > + tdx_caps = (struct tdx_capabilities) {
> > + .tdcs_nr_pages = tdsysinfo->tdcs_base_size / PAGE_SIZE,
> > + /*
> > + * TDVPS = TDVPR(4K page) + TDVPX(multiple 4K pages).
> > + * -1 for TDVPR.
> > + */
> > + .tdvpx_nr_pages = tdsysinfo->tdvps_base_size / PAGE_SIZE - 1,
> > + .attrs_fixed0 = tdsysinfo->attributes_fixed0,
> > + .attrs_fixed1 = tdsysinfo->attributes_fixed1,
> > + .xfam_fixed0 = tdsysinfo->xfam_fixed0,
> > + .xfam_fixed1 = tdsysinfo->xfam_fixed1,
> > + .nr_cpuid_configs = tdsysinfo->num_cpuid_config,
> > + };
> > + if (!memcpy(tdx_caps.cpuid_configs, tdsysinfo->cpuid_configs,
> > + tdsysinfo->num_cpuid_config *
> > + sizeof(struct tdx_cpuid_config)))
> > + return -EIO;
>
> Why introducing 'struct tdx_capabilities' and above code here in this patch?
>
> It's entirely not clear why the new structure is needed -- nothing mentioned in
> changelog, nor there's any comment. Please explain in the changelog or move
> this chunk to where it is needed.
>
> Technical side, is 'struct tdx_capabilities' really needed? Or is it just for
> convenience?
I made use of tdsysinfo where possible.
Because it's convenient to cache the number of tdcs_pages and tdvps_pages, it
renamed tdx_capabilities into tdx_info to keep those two. and Move it to the
patch that uses the value.
--
Isaku Yamahata <isaku.yamahata@...il.com>
Powered by blists - more mailing lists