Re: [PATCH v6 16/22] x86/virt/tdx: Update tdx_sysinfo and check features post-update

[Chao Gao]

>+int tdx_module_post_update(struct tdx_sys_info *info)
>+{
>+	struct tdx_sys_info_version *old, *new;
>+	int ret;
>+
>+	/* Shouldn't fail as the update has succeeded. */
>+	ret = get_tdx_sys_info(info);
>+	if (WARN_ONCE(ret, "version retrieval failed after update, replace the TDX module\n"))
>+		return ret;
>+
>+	old = &tdx_sysinfo.version;
>+	new = &info->version;
>+	pr_info("version %u.%u.%02u -> %u.%u.%02u\n", old->major_version,
>+						      old->minor_version,
>+						      old->update_version,
>+						      new->major_version,
>+						      new->minor_version,
>+						      new->update_version);
>+
>+	/*
>+	 * Blindly refreshing the entire tdx_sysinfo could disrupt running
>+	 * software, as it may subtly rely on the previous state unless
>+	 * proven otherwise.
>+	 *
>+	 * Only refresh version information (including handoff version)
>+	 * that does not affect functionality, and ignore all other
>+	 * changes.
>+	 */
>+	tdx_sysinfo.version	= info->version;
>+	tdx_sysinfo.handoff	= info->handoff;

Sashiko commented:
"""
Because stop_machine() has already completed in seamldr_install_module(),
other CPUs will have resumed execution by the time this is called.
Since tdx_sysinfo.version and tdx_sysinfo.handoff are multi-byte structures
and are updated here without holding a lock, could concurrent readers observe
torn reads if they access these fields simultaneously?
"""

This is valid. tdx_sysinfo.handoff has no concurrent readers. so, no fix is
needed.

tdx_sysinfo.version may be read by userspace via sysfs. However, major/minor
versions don't change across updates, so only update_version needs
READ/WRITE_ONCE() to prevent torn reads. I will apply this fix:

diff --git a/arch/x86/virt/vmx/tdx/tdx.c b/arch/x86/virt/vmx/tdx/tdx.c
index 432d80b21ef0..0e7668bf20a1 100644
--- a/arch/x86/virt/vmx/tdx/tdx.c
+++ b/arch/x86/virt/vmx/tdx/tdx.c
@@ -1276,7 +1276,7 @@ int tdx_module_post_update(struct tdx_sys_info *info)
         * that does not affect functionality, and ignore all other
         * changes.
         */
-       tdx_sysinfo.version     = info->version;
+       WRITE_ONCE(tdx_sysinfo.version.update_version, info->version.update_version);
        tdx_sysinfo.handoff     = info->handoff;
 
        if (!memcmp(&tdx_sysinfo, info, sizeof(*info)))
diff --git a/drivers/virt/coco/tdx-host/tdx-host.c b/drivers/virt/coco/tdx-host/tdx-host.c
index d4a552853021..43a55666145c 100644
--- a/drivers/virt/coco/tdx-host/tdx-host.c
+++ b/drivers/virt/coco/tdx-host/tdx-host.c
@@ -40,7 +40,7 @@ static ssize_t version_show(struct device *dev, struct device_attribute *attr,
 
        return sysfs_emit(buf, TDX_VERSION_FMT"\n", ver->major_version,
                                                    ver->minor_version,
-                                                   ver->update_version);
+                                                   READ_ONCE(ver->update_version));
 }
 static DEVICE_ATTR_RO(version);

>+
>+	if (!memcmp(&tdx_sysinfo, info, sizeof(*info)))
>+		return 0;
>+
>+	pr_info("TDX module features have changed after updates, but might not take effect.\n");
>+	pr_info("Please consider updating your BIOS to install the TDX module.\n");
>+	return 0;
>+}
>+
> static bool is_pamt_page(unsigned long phys)
> {
> 	struct tdmr_info_list *tdmr_list = &tdx_tdmr_list;
>diff --git a/arch/x86/virt/vmx/tdx/tdx.h b/arch/x86/virt/vmx/tdx/tdx.h
>index c62874b87d7a..f8686247c660 100644
>--- a/arch/x86/virt/vmx/tdx/tdx.h
>+++ b/arch/x86/virt/vmx/tdx/tdx.h
>@@ -4,6 +4,8 @@
> 
> #include <linux/bits.h>
> 
>+#include <asm/tdx_global_metadata.h>
>+
> /*
>  * This file contains both macros and data structures defined by the TDX
>  * architecture and Linux defined software data structures and functions.
>@@ -122,5 +124,6 @@ struct tdmr_info_list {
> 
> int tdx_module_shutdown(void);
> int tdx_module_run_update(void);
>+int tdx_module_post_update(struct tdx_sys_info *info);
> 
> #endif
>-- 
>2.47.3
>