| 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: <66b1a44236bf8_4fc72945a@dwillia2-xfh.jf.intel.com.notmuch>
Date: Mon, 5 Aug 2024 21:19:14 -0700
From: Dan Williams <dan.j.williams@...el.com>
To: Kai Huang <kai.huang@...el.com>, <dave.hansen@...el.com>,
<kirill.shutemov@...ux.intel.com>, <bp@...en8.de>, <tglx@...utronix.de>,
<peterz@...radead.org>, <mingo@...hat.com>, <hpa@...or.com>,
<seanjc@...gle.com>, <pbonzini@...hat.com>, <dan.j.williams@...el.com>
CC: <x86@...nel.org>, <kvm@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
<rick.p.edgecombe@...el.com>, <isaku.yamahata@...el.com>,
<chao.gao@...el.com>, <binbin.wu@...ux.intel.com>, <kai.huang@...el.com>
Subject: Re: [PATCH v2 08/10] x86/virt/tdx: Print TDX module basic information
Kai Huang wrote:
> Currently the kernel doesn't print any information regarding the TDX
> module itself, e.g. module version. In practice such information is
> useful, especially to the developers.
>
> For instance, there are a couple of use cases for dumping module basic
> information:
>
> 1) When something goes wrong around using TDX, the information like TDX
> module version, supported features etc could be helpful [1][2].
>
> 2) For Linux, when the user wants to update the TDX module, one needs to
> replace the old module in a specific location in the EFI partition
> with the new one so that after reboot the BIOS can load it. However,
> after kernel boots, currently the user has no way to verify it is
> indeed the new module that gets loaded and initialized (e.g., error
> could happen when replacing the old module). With the module version
> dumped the user can verify this easily.
>
> So dump the basic TDX module information:
>
> - TDX module version, and the build date.
> - TDX module type: Debug or Production.
> - TDX_FEATURES0: Supported TDX features.
>
> And dump the information right after reading global metadata, so that
> this information is printed no matter whether module initialization
> fails or not.
>
> The actual dmesg will look like:
>
> virt/tdx: Initializing TDX module: 1.5.00.00.0481 (build_date 20230323, Production module), TDX_FEATURES0 0xfbf
>
> Link: https://lore.kernel.org/lkml/e2d844ad-182a-4fc0-a06a-d609c9cbef74@suse.com/T/#m352829aedf6680d4628c7e40dc40b332eda93355 [1]
> Link: https://lore.kernel.org/lkml/e2d844ad-182a-4fc0-a06a-d609c9cbef74@suse.com/T/#m351ebcbc006d2e5bc3e7650206a087cb2708d451 [2]
> Signed-off-by: Kai Huang <kai.huang@...el.com>
> ---
>
> v1 -> v2 (Nikolay):
> - Change the format to dump TDX basic info.
> - Slightly improve changelog.
>
> ---
> arch/x86/virt/vmx/tdx/tdx.c | 64 +++++++++++++++++++++++++++++++++++++
> arch/x86/virt/vmx/tdx/tdx.h | 33 ++++++++++++++++++-
> 2 files changed, 96 insertions(+), 1 deletion(-)
>
> diff --git a/arch/x86/virt/vmx/tdx/tdx.c b/arch/x86/virt/vmx/tdx/tdx.c
> index 3253cdfa5207..5ac0c411f4f7 100644
> --- a/arch/x86/virt/vmx/tdx/tdx.c
> +++ b/arch/x86/virt/vmx/tdx/tdx.c
> @@ -319,6 +319,58 @@ static int stbuf_read_sysmd_multi(const struct field_mapping *fields,
> return 0;
> }
>
> +#define TD_SYSINFO_MAP_MOD_INFO(_field_id, _member) \
> + TD_SYSINFO_MAP(_field_id, struct tdx_sysinfo_module_info, _member)
> +
> +static int get_tdx_module_info(struct tdx_sysinfo_module_info *modinfo)
> +{
> + static const struct field_mapping fields[] = {
> + TD_SYSINFO_MAP_MOD_INFO(SYS_ATTRIBUTES, sys_attributes),
> + TD_SYSINFO_MAP_MOD_INFO(TDX_FEATURES0, tdx_features0),
> + };
> +
> + return stbuf_read_sysmd_multi(fields, ARRAY_SIZE(fields), modinfo);
> +}
> +
> +#define TD_SYSINFO_MAP_MOD_VERSION(_field_id, _member) \
> + TD_SYSINFO_MAP(_field_id, struct tdx_sysinfo_module_version, _member)
> +
> +static int get_tdx_module_version(struct tdx_sysinfo_module_version *modver)
> +{
> + static const struct field_mapping fields[] = {
> + TD_SYSINFO_MAP_MOD_VERSION(MAJOR_VERSION, major),
> + TD_SYSINFO_MAP_MOD_VERSION(MINOR_VERSION, minor),
> + TD_SYSINFO_MAP_MOD_VERSION(UPDATE_VERSION, update),
> + TD_SYSINFO_MAP_MOD_VERSION(INTERNAL_VERSION, internal),
> + TD_SYSINFO_MAP_MOD_VERSION(BUILD_NUM, build_num),
> + TD_SYSINFO_MAP_MOD_VERSION(BUILD_DATE, build_date),
> + };
> +
> + return stbuf_read_sysmd_multi(fields, ARRAY_SIZE(fields), modver);
Looks good if stbuf_read_sysmd_multi() is replaced with the work being
done internal to TD_SYSINFO_MAP_MOD_VERSION().
> +}
> +
> +static void print_basic_sysinfo(struct tdx_sysinfo *sysinfo)
> +{
> + struct tdx_sysinfo_module_version *modver = &sysinfo->module_version;
> + struct tdx_sysinfo_module_info *modinfo = &sysinfo->module_info;
> + bool debug = modinfo->sys_attributes & TDX_SYS_ATTR_DEBUG_MODULE;
Why is this casually checking for debug modules, but doing nothing with
that indication? Shouldn't the kernel have policy around whether it
wants to interoperate with a debug module? I would expect that kernel
operation with a debug module would need explicit opt-in consideration.
> +
> + /*
> + * TDX module version encoding:
> + *
> + * <major>.<minor>.<update>.<internal>.<build_num>
> + *
> + * When printed as text, <major> and <minor> are 1-digit,
> + * <update> and <internal> are 2-digits and <build_num>
> + * is 4-digits.
> + */
> + pr_info("Initializing TDX module: %u.%u.%02u.%02u.%04u (build_date %u, %s module), TDX_FEATURES0 0x%llx\n",
> + modver->major, modver->minor, modver->update,
> + modver->internal, modver->build_num,
> + modver->build_date, debug ? "Debug" : "Production",
> + modinfo->tdx_features0);
Another nice thing about json scripting is that this flag fields could
be pretty-printed with symbolic names for the flags, but that can come
later.
> +}
> +
> #define TD_SYSINFO_MAP_TDMR_INFO(_field_id, _member) \
> TD_SYSINFO_MAP(_field_id, struct tdx_sysinfo_tdmr_info, _member)
>
> @@ -339,6 +391,16 @@ static int get_tdx_tdmr_sysinfo(struct tdx_sysinfo_tdmr_info *tdmr_sysinfo)
>
> static int get_tdx_sysinfo(struct tdx_sysinfo *sysinfo)
> {
> + int ret;
> +
> + ret = get_tdx_module_info(&sysinfo->module_info);
> + if (ret)
> + return ret;
> +
> + ret = get_tdx_module_version(&sysinfo->module_version);
> + if (ret)
> + return ret;
> +
> return get_tdx_tdmr_sysinfo(&sysinfo->tdmr_info);
> }
>
> @@ -1121,6 +1183,8 @@ static int init_tdx_module(void)
> if (ret)
> return ret;
>
> + print_basic_sysinfo(&sysinfo);
> +
> /*
> * To keep things simple, assume that all TDX-protected memory
> * will come from the page allocator. Make sure all pages in the
> diff --git a/arch/x86/virt/vmx/tdx/tdx.h b/arch/x86/virt/vmx/tdx/tdx.h
> index b5eb7c35f1dc..861ddf2c2e88 100644
> --- a/arch/x86/virt/vmx/tdx/tdx.h
> +++ b/arch/x86/virt/vmx/tdx/tdx.h
> @@ -31,6 +31,15 @@
> *
> * See the "global_metadata.json" in the "TDX 1.5 ABI definitions".
> */
> +#define MD_FIELD_ID_SYS_ATTRIBUTES 0x0A00000200000000ULL
> +#define MD_FIELD_ID_TDX_FEATURES0 0x0A00000300000008ULL
> +#define MD_FIELD_ID_BUILD_DATE 0x8800000200000001ULL
> +#define MD_FIELD_ID_BUILD_NUM 0x8800000100000002ULL
> +#define MD_FIELD_ID_MINOR_VERSION 0x0800000100000003ULL
> +#define MD_FIELD_ID_MAJOR_VERSION 0x0800000100000004ULL
> +#define MD_FIELD_ID_UPDATE_VERSION 0x0800000100000005ULL
> +#define MD_FIELD_ID_INTERNAL_VERSION 0x0800000100000006ULL
This is where I would rather not take your word for it, or go review
these constants myself if these were autogenerated from parsing json.
> +
> #define MD_FIELD_ID_MAX_TDMRS 0x9100000100000008ULL
> #define MD_FIELD_ID_MAX_RESERVED_PER_TDMR 0x9100000100000009ULL
> #define MD_FIELD_ID_PAMT_4K_ENTRY_SIZE 0x9100000100000010ULL
> @@ -124,8 +133,28 @@ struct tdmr_info_list {
> *
> * Note not all metadata fields in each class are defined, only those
> * used by the kernel are.
> + *
> + * Also note the "bit definitions" are architectural.
> */
>
> +/* Class "TDX Module Info" */
> +struct tdx_sysinfo_module_info {
This name feels too generic, perhaps 'tdx_sys_info_features' makes it
clearer?
> + u32 sys_attributes;
> + u64 tdx_features0;
> +};
> +
> +#define TDX_SYS_ATTR_DEBUG_MODULE 0x1
> +
> +/* Class "TDX Module Version" */
> +struct tdx_sysinfo_module_version {
> + u16 major;
> + u16 minor;
> + u16 update;
> + u16 internal;
> + u16 build_num;
> + u32 build_date;
> +};
> +
> /* Class "TDMR Info" */
> struct tdx_sysinfo_tdmr_info {
> u16 max_tdmrs;
> @@ -134,7 +163,9 @@ struct tdx_sysinfo_tdmr_info {
> };
>
> struct tdx_sysinfo {
> - struct tdx_sysinfo_tdmr_info tdmr_info;
> + struct tdx_sysinfo_module_info module_info;
> + struct tdx_sysinfo_module_version module_version;
> + struct tdx_sysinfo_tdmr_info tdmr_info;
Compare that to:
struct tdx_sys_info {
struct tdx_sys_info_features features;
struct tdx_sys_info_version version;
struct tdx_sys_info_tdmr tdmr;
};
...and tell me which oine is easier to read.
Powered by blists - more mailing lists