[PATCH] x86/kvm/tdx: Save %rbp in TDX_MODULE_CALL

[PATCH] x86/kvm/tdx: Save %rbp in TDX_MODULE_CALL

[Juergen Gross]

While testing TDX host support patches, a crash of the host has been
observed a few instructions after doing a seamcall. Reason was a
clobbered %rbp (set to 0), which occurred in spite of the TDX module
offering the feature NOT to modify %rbp across TDX module calls.

In order not having to build the host kernel with CONFIG_FRAME_POINTER,
save %rbp across a seamcall/tdcall.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
 arch/x86/virt/vmx/tdx/tdxcall.S | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/arch/x86/virt/vmx/tdx/tdxcall.S b/arch/x86/virt/vmx/tdx/tdxcall.S
index 016a2a1ec1d6..68728acf0d3a 100644
--- a/arch/x86/virt/vmx/tdx/tdxcall.S
+++ b/arch/x86/virt/vmx/tdx/tdxcall.S
@@ -44,6 +44,10 @@
  */
 .macro TDX_MODULE_CALL host:req ret=0 saved=0
 	FRAME_BEGIN
+#ifndef CONFIG_FRAME_POINTER
+	/* Buggy firmware sometimes clobbers %rbp, so save it. */
+	pushq	%rbp
+#endif
 
 	/* Move Leaf ID to RAX */
 	mov %rdi, %rax
@@ -187,6 +191,9 @@
 	popq	%rbx
 .endif	/* \saved */
 
+#ifndef CONFIG_FRAME_POINTER
+	popq	%rbp
+#endif
 	FRAME_END
 	RET
 
-- 
2.35.3

Re: [PATCH] x86/kvm/tdx: Save %rbp in TDX_MODULE_CALL

[Kirill A. Shutemov]

On Fri, May 17, 2024 at 02:14:50PM +0200, Juergen Gross wrote:
> While testing TDX host support patches, a crash of the host has been
> observed a few instructions after doing a seamcall. Reason was a
> clobbered %rbp (set to 0), which occurred in spite of the TDX module
> offering the feature NOT to modify %rbp across TDX module calls.
> 
> In order not having to build the host kernel with CONFIG_FRAME_POINTER,
> save %rbp across a seamcall/tdcall.

There's a feature in TDX module 1.5 that prevents RBP modification across
TDH.VP.ENTER SEAMCALL. See NO_RBP_MOD in TDX Module 1.5 ABI spec.

I think it has to be enabled for all TDs and TDX modules that don't
support it need to be rejected.

-- 
  Kiryl Shutsemau / Kirill A. Shutemov

Re: [PATCH] x86/kvm/tdx: Save %rbp in TDX_MODULE_CALL

[Juergen Gross]

[-- Attachment #1.1.1: Type: text/plain, Size: 2017 bytes --]

On 17.05.24 15:55, Kirill A. Shutemov wrote:
> On Fri, May 17, 2024 at 02:14:50PM +0200, Juergen Gross wrote:
>> While testing TDX host support patches, a crash of the host has been
>> observed a few instructions after doing a seamcall. Reason was a
>> clobbered %rbp (set to 0), which occurred in spite of the TDX module
>> offering the feature NOT to modify %rbp across TDX module calls.
>>
>> In order not having to build the host kernel with CONFIG_FRAME_POINTER,
>> save %rbp across a seamcall/tdcall.
> 
> There's a feature in TDX module 1.5 that prevents RBP modification across
> TDH.VP.ENTER SEAMCALL. See NO_RBP_MOD in TDX Module 1.5 ABI spec.
> 
> I think it has to be enabled for all TDs and TDX modules that don't
> support it need to be rejected.
> 

Yes, I know. I'm using the patch series:

   [PATCH v19 000/130] KVM TDX basic feature support

which I think does exactly that (see setup_tdparams() and tdx_module_setup()).

Nevertheless the clobbering happened, and saving/restoring %rbp made the
issue to go away. I suspect there is a path left still clobbering %rbp.

I was testing on an Emerald Rapids system:

# lscpu
Architecture:             x86_64
   CPU op-mode(s):         32-bit, 64-bit
   Address sizes:          47 bits physical, 57 bits virtual
   Byte Order:             Little Endian
CPU(s):                   256
   On-line CPU(s) list:    0-255
Vendor ID:                GenuineIntel
   BIOS Vendor ID:         Intel(R) Corporation
   Model name:             INTEL(R) XEON(R) PLATINUM 8592+
     BIOS Model name:      INTEL(R) XEON(R) PLATINUM 8592+  CPU @ 1.9GHz
     BIOS CPU family:      179
     CPU family:           6
     Model:                207
     Thread(s) per core:   2
     Core(s) per socket:   64
     Socket(s):            2
     Stepping:             2
...

BIOS version as printed during boot:

[    0.000000] DMI: Intel Corporation D50DNP/D50DNP, BIOS 
SE5C7411.86B.9535.D04.2312270518 12/27/2023


Juergen

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3743 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

Re: [PATCH] x86/kvm/tdx: Save %rbp in TDX_MODULE_CALL

[Kirill A. Shutemov]

On Fri, May 17, 2024 at 04:08:03PM +0200, Juergen Gross wrote:
> On 17.05.24 15:55, Kirill A. Shutemov wrote:
> > On Fri, May 17, 2024 at 02:14:50PM +0200, Juergen Gross wrote:
> > > While testing TDX host support patches, a crash of the host has been
> > > observed a few instructions after doing a seamcall. Reason was a
> > > clobbered %rbp (set to 0), which occurred in spite of the TDX module
> > > offering the feature NOT to modify %rbp across TDX module calls.
> > > 
> > > In order not having to build the host kernel with CONFIG_FRAME_POINTER,
> > > save %rbp across a seamcall/tdcall.
> > 
> > There's a feature in TDX module 1.5 that prevents RBP modification across
> > TDH.VP.ENTER SEAMCALL. See NO_RBP_MOD in TDX Module 1.5 ABI spec.
> > 
> > I think it has to be enabled for all TDs and TDX modules that don't
> > support it need to be rejected.
> > 
> 
> Yes, I know. I'm using the patch series:
> 
>   [PATCH v19 000/130] KVM TDX basic feature support
> 
> which I think does exactly that (see setup_tdparams() and tdx_module_setup()).

Looks like the check is broken:

https://lore.kernel.org/all/46mh5hinsv5mup2x7jv4iu2floxmajo2igrxb3haru3cgjukbg@v44nspjozm4h/

> Nevertheless the clobbering happened, and saving/restoring %rbp made the
> issue to go away. I suspect there is a path left still clobbering %rbp.

What is your TDX module version? My guess is that NOM_RBP_MOD is not
supported by it and given that the check is broken nobody enforces it.

-- 
  Kiryl Shutsemau / Kirill A. Shutemov

Re: [PATCH] x86/kvm/tdx: Save %rbp in TDX_MODULE_CALL

[Kirill A. Shutemov]

On Fri, May 17, 2024 at 05:39:51PM +0300, Kirill A. Shutemov wrote:
> On Fri, May 17, 2024 at 04:08:03PM +0200, Juergen Gross wrote:
> > On 17.05.24 15:55, Kirill A. Shutemov wrote:
> > > On Fri, May 17, 2024 at 02:14:50PM +0200, Juergen Gross wrote:
> > > > While testing TDX host support patches, a crash of the host has been
> > > > observed a few instructions after doing a seamcall. Reason was a
> > > > clobbered %rbp (set to 0), which occurred in spite of the TDX module
> > > > offering the feature NOT to modify %rbp across TDX module calls.
> > > > 
> > > > In order not having to build the host kernel with CONFIG_FRAME_POINTER,
> > > > save %rbp across a seamcall/tdcall.
> > > 
> > > There's a feature in TDX module 1.5 that prevents RBP modification across
> > > TDH.VP.ENTER SEAMCALL. See NO_RBP_MOD in TDX Module 1.5 ABI spec.
> > > 
> > > I think it has to be enabled for all TDs and TDX modules that don't
> > > support it need to be rejected.
> > > 
> > 
> > Yes, I know. I'm using the patch series:
> > 
> >   [PATCH v19 000/130] KVM TDX basic feature support
> > 
> > which I think does exactly that (see setup_tdparams() and tdx_module_setup()).
> 
> Looks like the check is broken:
> 
> https://lore.kernel.org/all/46mh5hinsv5mup2x7jv4iu2floxmajo2igrxb3haru3cgjukbg@v44nspjozm4h/

Err.. I think I confused myself. Please ignore.

-- 
  Kiryl Shutsemau / Kirill A. Shutemov

Re: [PATCH] x86/kvm/tdx: Save %rbp in TDX_MODULE_CALL

[Juergen Gross]

[-- Attachment #1.1.1: Type: text/plain, Size: 1843 bytes --]

On 17.05.24 16:39, Kirill A. Shutemov wrote:
> On Fri, May 17, 2024 at 04:08:03PM +0200, Juergen Gross wrote:
>> On 17.05.24 15:55, Kirill A. Shutemov wrote:
>>> On Fri, May 17, 2024 at 02:14:50PM +0200, Juergen Gross wrote:
>>>> While testing TDX host support patches, a crash of the host has been
>>>> observed a few instructions after doing a seamcall. Reason was a
>>>> clobbered %rbp (set to 0), which occurred in spite of the TDX module
>>>> offering the feature NOT to modify %rbp across TDX module calls.
>>>>
>>>> In order not having to build the host kernel with CONFIG_FRAME_POINTER,
>>>> save %rbp across a seamcall/tdcall.
>>>
>>> There's a feature in TDX module 1.5 that prevents RBP modification across
>>> TDH.VP.ENTER SEAMCALL. See NO_RBP_MOD in TDX Module 1.5 ABI spec.
>>>
>>> I think it has to be enabled for all TDs and TDX modules that don't
>>> support it need to be rejected.
>>>
>>
>> Yes, I know. I'm using the patch series:
>>
>>    [PATCH v19 000/130] KVM TDX basic feature support
>>
>> which I think does exactly that (see setup_tdparams() and tdx_module_setup()).
> 
> Looks like the check is broken:
> 
> https://lore.kernel.org/all/46mh5hinsv5mup2x7jv4iu2floxmajo2igrxb3haru3cgjukbg@v44nspjozm4h/
> 
>> Nevertheless the clobbering happened, and saving/restoring %rbp made the
>> issue to go away. I suspect there is a path left still clobbering %rbp.
> 
> What is your TDX module version? My guess is that NOM_RBP_MOD is not
> supported by it and given that the check is broken nobody enforces it.

Just another data point: Before using this machine I was testing on
another one with older firmware. That one really didn't support NOM_RBP_MOD
and I needed to build the kernel with CONFIG_FRAME_POINTER enabled to get
past the check you are mentioning above.


Juergen

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3743 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

Re: [PATCH] x86/kvm/tdx: Save %rbp in TDX_MODULE_CALL

[Dave Hansen]

On 5/17/24 07:44, Juergen Gross wrote:
> Just another data point: Before using this machine I was testing on
> another one with older firmware. That one really didn't support NOM_RBP_MOD
> and I needed to build the kernel with CONFIG_FRAME_POINTER enabled to get
> past the check you are mentioning above.

For all intents and purposes, the modules that intentionally clobber RBP
don't support Linux. If buggy modules are accidentally clobbering RBP,
we can debate how much the kernel should bend over to accommodate them,
but my preference would be to ignore them.

I'd much rather put a deny list in the kernel than try to tolerate RBP
clobbering universally.

Re: [PATCH] x86/kvm/tdx: Save %rbp in TDX_MODULE_CALL

[Jürgen Groß]

On 17.05.24 17:16, Dave Hansen wrote:
> On 5/17/24 07:44, Juergen Gross wrote:
>> Just another data point: Before using this machine I was testing on
>> another one with older firmware. That one really didn't support NOM_RBP_MOD
>> and I needed to build the kernel with CONFIG_FRAME_POINTER enabled to get
>> past the check you are mentioning above.
> 
> For all intents and purposes, the modules that intentionally clobber RBP
> don't support Linux. If buggy modules are accidentally clobbering RBP,
> we can debate how much the kernel should bend over to accommodate them,
> but my preference would be to ignore them.
> 
> I'd much rather put a deny list in the kernel than try to tolerate RBP
> clobbering universally.

Would you be fine with adding a new X86_FEATURE (or BUG?) allowing to switch
RBP save/restore via ALTERNATIVE, controlled by a command line option?

Or maybe by adding a new CONFIG_TDX_MODULE_CAN_CLOBBER_RBP (probably using
a shorter name) option?

TBH I'm slightly puzzled that the firmware I'm using could make it outside
Intel. I'm fearing this might happen again.


Juergen

Re: [PATCH] x86/kvm/tdx: Save %rbp in TDX_MODULE_CALL

[Dave Hansen]

On 5/17/24 08:27, Jürgen Groß wrote:
> On 17.05.24 17:16, Dave Hansen wrote:
>> On 5/17/24 07:44, Juergen Gross wrote:
>>> Just another data point: Before using this machine I was testing on
>>> another one with older firmware. That one really didn't support
>>> NOM_RBP_MOD
>>> and I needed to build the kernel with CONFIG_FRAME_POINTER enabled to
>>> get
>>> past the check you are mentioning above.
>>
>> For all intents and purposes, the modules that intentionally clobber RBP
>> don't support Linux. If buggy modules are accidentally clobbering RBP,
>> we can debate how much the kernel should bend over to accommodate them,
>> but my preference would be to ignore them.
>>
>> I'd much rather put a deny list in the kernel than try to tolerate RBP
>> clobbering universally.
> 
> Would you be fine with adding a new X86_FEATURE (or BUG?) allowing
> to switch RBP save/restore via ALTERNATIVE, controlled by a command
> line option?
> 
> Or maybe by adding a new CONFIG_TDX_MODULE_CAN_CLOBBER_RBP (probably
> using a shorter name) option?

As a last resort maybe.

> TBH I'm slightly puzzled that the firmware I'm using could make it
> outside Intel. I'm fearing this might happen again.

You're puzzled that the firmware is either old buggy or both? Huh.

Intel ships all kinds of crazy pre-production stuff as development
platforms. Let's make sure we know what you've got before we go tearing
up mainline for it.

Because if the options are:

 1. Maintain code in mainline until the day I die^Wretire

or

 2. Get Jürgen a BIOS update so he stops sending patches

... it's kinda an easy choice. ;)

Re: [PATCH] x86/kvm/tdx: Save %rbp in TDX_MODULE_CALL

[Juergen Gross]

[-- Attachment #1.1.1: Type: text/plain, Size: 1953 bytes --]

On 17.05.24 17:43, Dave Hansen wrote:
> On 5/17/24 08:27, Jürgen Groß wrote:
>> On 17.05.24 17:16, Dave Hansen wrote:
>>> On 5/17/24 07:44, Juergen Gross wrote:
>>>> Just another data point: Before using this machine I was testing on
>>>> another one with older firmware. That one really didn't support
>>>> NOM_RBP_MOD
>>>> and I needed to build the kernel with CONFIG_FRAME_POINTER enabled to
>>>> get
>>>> past the check you are mentioning above.
>>>
>>> For all intents and purposes, the modules that intentionally clobber RBP
>>> don't support Linux. If buggy modules are accidentally clobbering RBP,
>>> we can debate how much the kernel should bend over to accommodate them,
>>> but my preference would be to ignore them.
>>>
>>> I'd much rather put a deny list in the kernel than try to tolerate RBP
>>> clobbering universally.
>>
>> Would you be fine with adding a new X86_FEATURE (or BUG?) allowing
>> to switch RBP save/restore via ALTERNATIVE, controlled by a command
>> line option?
>>
>> Or maybe by adding a new CONFIG_TDX_MODULE_CAN_CLOBBER_RBP (probably
>> using a shorter name) option?
> 
> As a last resort maybe.
> 
>> TBH I'm slightly puzzled that the firmware I'm using could make it
>> outside Intel. I'm fearing this might happen again.
> 
> You're puzzled that the firmware is either old buggy or both? Huh.
> 
> Intel ships all kinds of crazy pre-production stuff as development
> platforms. Let's make sure we know what you've got before we go tearing
> up mainline for it.
> 
> Because if the options are:
> 
>   1. Maintain code in mainline until the day I die^Wretire
> 
> or
> 
>   2. Get Jürgen a BIOS update so he stops sending patches
> 
> .. it's kinda an easy choice. ;)
> 

:-)

Is the BIOS version printed at boot enough to see what I have?

[    0.000000] DMI: Intel Corporation D50DNP/D50DNP, BIOS 
SE5C7411.86B.9535.D04.2312270518 12/27/2023


Juergen

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3743 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

Re: [PATCH] x86/kvm/tdx: Save %rbp in TDX_MODULE_CALL

[Dave Hansen]

On 5/17/24 08:48, Juergen Gross wrote:
> Is the BIOS version printed at boot enough to see what I have?
> 
> [    0.000000] DMI: Intel Corporation D50DNP/D50DNP, BIOS
> SE5C7411.86B.9535.D04.2312270518 12/27/2023

I honestly don't know.

What we actually need is the TDX module version. I'm not sure how
tightly tied the TDX module is to the BIOS version. I suspect that
they're actually completely independent.

Once we have the specific TDX module version, we can go ask the folks
who write it if there were any RBP clobbering bugs.

Re: [PATCH] x86/kvm/tdx: Save %rbp in TDX_MODULE_CALL

[Juergen Gross]

[-- Attachment #1.1.1: Type: text/plain, Size: 675 bytes --]

On 17.05.24 17:52, Dave Hansen wrote:
> On 5/17/24 08:48, Juergen Gross wrote:
>> Is the BIOS version printed at boot enough to see what I have?
>>
>> [    0.000000] DMI: Intel Corporation D50DNP/D50DNP, BIOS
>> SE5C7411.86B.9535.D04.2312270518 12/27/2023
> 
> I honestly don't know.
> 
> What we actually need is the TDX module version. I'm not sure how
> tightly tied the TDX module is to the BIOS version. I suspect that
> they're actually completely independent.
> 
> Once we have the specific TDX module version, we can go ask the folks
> who write it if there were any RBP clobbering bugs.
> 

Okay, how to get the TDX module version?


Juergen

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3743 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

Re: [PATCH] x86/kvm/tdx: Save %rbp in TDX_MODULE_CALL

[Dave Hansen]

On 5/17/24 08:58, Juergen Gross wrote:
> On 17.05.24 17:52, Dave Hansen wrote:
...
>> Once we have the specific TDX module version, we can go ask the folks
>> who write it if there were any RBP clobbering bugs.
> 
> Okay, how to get the TDX module version?

You need something like this:

> https://lore.kernel.org/all/20231012134136.1310650-1-yi.sun@intel.com/

... and yeah, this needs to be upstream.

Re: [PATCH] x86/kvm/tdx: Save %rbp in TDX_MODULE_CALL

[Huang, Kai]

[-- Attachment #1: Type: text/plain, Size: 1734 bytes --]

On Fri, 2024-05-17 at 09:48 -0700, Dave Hansen wrote:
> On 5/17/24 08:58, Juergen Gross wrote:
> > On 17.05.24 17:52, Dave Hansen wrote:
> ..
> > > Once we have the specific TDX module version, we can go ask the folks
> > > who write it if there were any RBP clobbering bugs.
> > 
> > Okay, how to get the TDX module version?
> 
> You need something like this:
> 
> > https://lore.kernel.org/all/20231012134136.1310650-1-yi.sun@intel.com/

This one prints TDX version info in the TDX guest, but not host.

The attached diff prints the TDX version (something like below) during
module initialization, and should meet Juergen's needs for temporary use:

[  113.543538] virt/tdx: module verson: major 1, minor 5, internal 0

> 
> .. and yeah, this needs to be upstream.
> 

From this thread I think it makes sense to add code to the TDX host code
to print the TDX version during module initialization.  I'll start to work
on this.

One thing is from the spec TDX has "4 versions": major, minor, update,
internal.  They are all 16-bit, and the overall version can be written in:

	<Major>.<Minor>.<Update>.<Internal>, e.g., 1.5.05.01

(see TDX module 1.5 API spec, section 3.3.2 "TDX Module Version".)

The attached diff only prints major, minor and internal, but leaves the
update out because I believe it is for module runtime update (yet to
confirm).

Given there are 4 versions, I think it makes sense to implement reading
them based on this patchset ...

https://lore.kernel.org/kvm/6940c326-bfca-4c67-badf-ab5c086bf492@intel.com/T/

... which extends the global metadata reading code to support any
arbitrary struct and all element sizes (although all 4 versions are 16-
bit)?





[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: print_tdx_version.diff --]
[-- Type: text/x-patch; name="print_tdx_version.diff", Size: 1584 bytes --]

diff --git a/arch/x86/virt/vmx/tdx/tdx.c b/arch/x86/virt/vmx/tdx/tdx.c
index 4d6826a76f78..f105214e36a3 100644
--- a/arch/x86/virt/vmx/tdx/tdx.c
+++ b/arch/x86/virt/vmx/tdx/tdx.c
@@ -1097,6 +1097,27 @@ static int init_tdmrs(struct tdmr_info_list *tdmr_list)
 	return 0;
 }
 
+static void print_tdx_version(void)
+{
+	u64 major, minor, internal;
+	int ret;
+
+	ret = read_sys_metadata_field(MD_FIELD_ID_MAJOR_VERSION, &major);
+	if (ret)
+		return;
+
+	ret = read_sys_metadata_field(MD_FIELD_ID_MINOR_VERSION, &minor);
+	if (ret)
+		return;
+
+	ret = read_sys_metadata_field(MD_FIELD_ID_INTERNAL_VERSION, &internal);
+	if (ret)
+		return;
+
+	pr_info("module verson: major %u, minor %u, internal %u\n",
+			(u16)major, (u16)minor, (u16)internal);
+}
+
 static int init_tdx_module(void)
 {
 	struct tdx_tdmr_sysinfo tdmr_sysinfo;
@@ -1155,6 +1176,9 @@ static int init_tdx_module(void)
 	 * Lock out memory hotplug code while building it.
 	 */
 	put_online_mems();
+
+	print_tdx_version();
+
 	return ret;
 
 err_reset_pamts:
diff --git a/arch/x86/virt/vmx/tdx/tdx.h b/arch/x86/virt/vmx/tdx/tdx.h
index b701f69485d3..ae8a96e0f53c 100644
--- a/arch/x86/virt/vmx/tdx/tdx.h
+++ b/arch/x86/virt/vmx/tdx/tdx.h
@@ -37,6 +37,10 @@
 #define MD_FIELD_ID_PAMT_2M_ENTRY_SIZE		0x9100000100000011ULL
 #define MD_FIELD_ID_PAMT_1G_ENTRY_SIZE		0x9100000100000012ULL
 
+#define MD_FIELD_ID_MINOR_VERSION		0x0800000100000003ULL
+#define MD_FIELD_ID_MAJOR_VERSION		0x0800000100000004ULL
+#define MD_FIELD_ID_INTERNAL_VERSION		0x0800000100000006ULL
+
 /*
  * Sub-field definition of metadata field ID.
  *

Re: [PATCH] x86/kvm/tdx: Save %rbp in TDX_MODULE_CALL

[Jürgen Groß]

On 20.05.24 13:54, Huang, Kai wrote:
> On Fri, 2024-05-17 at 09:48 -0700, Dave Hansen wrote:
>> On 5/17/24 08:58, Juergen Gross wrote:
>>> On 17.05.24 17:52, Dave Hansen wrote:
>> ..
>>>> Once we have the specific TDX module version, we can go ask the folks
>>>> who write it if there were any RBP clobbering bugs.
>>>
>>> Okay, how to get the TDX module version?
>>
>> You need something like this:
>>
>>> https://lore.kernel.org/all/20231012134136.1310650-1-yi.sun@intel.com/
> 
> This one prints TDX version info in the TDX guest, but not host.
> 
> The attached diff prints the TDX version (something like below) during
> module initialization, and should meet Juergen's needs for temporary use:
> 
> [  113.543538] virt/tdx: module verson: major 1, minor 5, internal 0
> 
>>
>> .. and yeah, this needs to be upstream.
>>
> 
>  From this thread I think it makes sense to add code to the TDX host code
> to print the TDX version during module initialization.  I'll start to work
> on this.
> 
> One thing is from the spec TDX has "4 versions": major, minor, update,
> internal.  They are all 16-bit, and the overall version can be written in:
> 
> 	<Major>.<Minor>.<Update>.<Internal>, e.g., 1.5.05.01
> 
> (see TDX module 1.5 API spec, section 3.3.2 "TDX Module Version".)
> 
> The attached diff only prints major, minor and internal, but leaves the
> update out because I believe it is for module runtime update (yet to
> confirm).
> 
> Given there are 4 versions, I think it makes sense to implement reading
> them based on this patchset ...
> 
> https://lore.kernel.org/kvm/6940c326-bfca-4c67-badf-ab5c086bf492@intel.com/T/
> 
> ... which extends the global metadata reading code to support any
> arbitrary struct and all element sizes (although all 4 versions are 16-
> bit)?

With that I got:

[   29.328484] virt/tdx: module verson: major 1, minor 5, internal 0


Juergen

Re: [PATCH] x86/kvm/tdx: Save %rbp in TDX_MODULE_CALL

[Huang, Kai]

On Thu, 2024-05-23 at 07:56 +0200, Jürgen Groß wrote:
> On 20.05.24 13:54, Huang, Kai wrote:
> > On Fri, 2024-05-17 at 09:48 -0700, Dave Hansen wrote:
> > > On 5/17/24 08:58, Juergen Gross wrote:
> > > > On 17.05.24 17:52, Dave Hansen wrote:
> > > ..
> > > > > Once we have the specific TDX module version, we can go ask the folks
> > > > > who write it if there were any RBP clobbering bugs.
> > > > 
> > > > Okay, how to get the TDX module version?
> > > 
> > > You need something like this:
> > > 
> > > > https://lore.kernel.org/all/20231012134136.1310650-1-yi.sun@intel.com/
> > 
> > This one prints TDX version info in the TDX guest, but not host.
> > 
> > The attached diff prints the TDX version (something like below) during
> > module initialization, and should meet Juergen's needs for temporary use:
> > 
> > [  113.543538] virt/tdx: module verson: major 1, minor 5, internal 0
> > 
> > > 
> > > .. and yeah, this needs to be upstream.
> > > 
> > 
> >  From this thread I think it makes sense to add code to the TDX host code
> > to print the TDX version during module initialization.  I'll start to work
> > on this.
> > 
> > One thing is from the spec TDX has "4 versions": major, minor, update,
> > internal.  They are all 16-bit, and the overall version can be written in:
> > 
> > 	<Major>.<Minor>.<Update>.<Internal>, e.g., 1.5.05.01
> > 
> > (see TDX module 1.5 API spec, section 3.3.2 "TDX Module Version".)
> > 
> > The attached diff only prints major, minor and internal, but leaves the
> > update out because I believe it is for module runtime update (yet to
> > confirm).
> > 
> > Given there are 4 versions, I think it makes sense to implement reading
> > them based on this patchset ...
> > 
> > https://lore.kernel.org/kvm/6940c326-bfca-4c67-badf-ab5c086bf492@intel.com/T/
> > 
> > ... which extends the global metadata reading code to support any
> > arbitrary struct and all element sizes (although all 4 versions are 16-
> > bit)?
> 
> With that I got:
> 
> [   29.328484] virt/tdx: module verson: major 1, minor 5, internal 0
> 
> 

Let me check TDX module guys on this and get back to you.

Re: [PATCH] x86/kvm/tdx: Save %rbp in TDX_MODULE_CALL

[Huang, Kai]

[-- Attachment #1: Type: text/plain, Size: 3074 bytes --]

On Thu, 2024-05-23 at 10:30 +0000, Huang, Kai wrote:
> On Thu, 2024-05-23 at 07:56 +0200, Jürgen Groß wrote:
> > On 20.05.24 13:54, Huang, Kai wrote:
> > > On Fri, 2024-05-17 at 09:48 -0700, Dave Hansen wrote:
> > > > On 5/17/24 08:58, Juergen Gross wrote:
> > > > > On 17.05.24 17:52, Dave Hansen wrote:
> > > > ..
> > > > > > Once we have the specific TDX module version, we can go ask the folks
> > > > > > who write it if there were any RBP clobbering bugs.
> > > > > 
> > > > > Okay, how to get the TDX module version?
> > > > 
> > > > You need something like this:
> > > > 
> > > > > https://lore.kernel.org/all/20231012134136.1310650-1-yi.sun@intel.com/
> > > 
> > > This one prints TDX version info in the TDX guest, but not host.
> > > 
> > > The attached diff prints the TDX version (something like below) during
> > > module initialization, and should meet Juergen's needs for temporary use:
> > > 
> > > [  113.543538] virt/tdx: module verson: major 1, minor 5, internal 0
> > > 
> > > > 
> > > > .. and yeah, this needs to be upstream.
> > > > 
> > > 
> > >  From this thread I think it makes sense to add code to the TDX host code
> > > to print the TDX version during module initialization.  I'll start to work
> > > on this.
> > > 
> > > One thing is from the spec TDX has "4 versions": major, minor, update,
> > > internal.  They are all 16-bit, and the overall version can be written in:
> > > 
> > > 	<Major>.<Minor>.<Update>.<Internal>, e.g., 1.5.05.01
> > > 
> > > (see TDX module 1.5 API spec, section 3.3.2 "TDX Module Version".)
> > > 
> > > The attached diff only prints major, minor and internal, but leaves the
> > > update out because I believe it is for module runtime update (yet to
> > > confirm).
> > > 
> > > Given there are 4 versions, I think it makes sense to implement reading
> > > them based on this patchset ...
> > > 
> > > https://lore.kernel.org/kvm/6940c326-bfca-4c67-badf-ab5c086bf492@intel.com/T/
> > > 
> > > ... which extends the global metadata reading code to support any
> > > arbitrary struct and all element sizes (although all 4 versions are 16-
> > > bit)?
> > 
> > With that I got:
> > 
> > [   29.328484] virt/tdx: module verson: major 1, minor 5, internal 0
> > 
> > 
> 
> Let me check TDX module guys on this and get back to you.

Hi Jurgen,

I was told the module starting with "1.5.06." has NO_RBP_MOD support.

And I think I was wrong about the <update> part of the version, and we
need that to determine the third part of the module version.

I was also told the 1.5.06 module hasn't been released to public yet, so I
guess your module doesn't support it.

I did another patch (attached) to check NO_RBP_MOD and reject module
initialization if it is not supported, and print out module version:

[  146.566641] virt/tdx: NO_RBP_MOD feature is not supported
[  146.572797] virt/tdx: module verson: 1.5.0.0
[  146.577731] virt/tdx: module initialization failed (-22)

You can have another try to verify at your side, if that helps.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: check_no_rbp_mod.diff --]
[-- Type: text/x-patch; name="check_no_rbp_mod.diff", Size: 2708 bytes --]

commit e3907269b34e4f488284f35716f231da7bc3e6b3
Author: Kai Huang <kai.huang@intel.com>
Date:   Thu May 23 23:17:28 2024 +1200

    x86/virt/tdx: check NO_RBP_MOD and print module version
    
    Signed-off-by: Kai Huang <kai.huang@intel.com>

diff --git a/arch/x86/virt/vmx/tdx/tdx.c b/arch/x86/virt/vmx/tdx/tdx.c
index 4d6826a76f78..ffc1c27b9134 100644
--- a/arch/x86/virt/vmx/tdx/tdx.c
+++ b/arch/x86/virt/vmx/tdx/tdx.c
@@ -1097,11 +1097,60 @@ static int init_tdmrs(struct tdmr_info_list *tdmr_list)
 	return 0;
 }
 
+static int check_module_compatibility(void)
+{
+	u64 tdx_features0;
+	int ret;
+
+	ret = read_sys_metadata_field(MD_FIELD_ID_TDX_FEATURES0,
+			&tdx_features0);
+
+	if (ret)
+		return ret;
+
+	if (!(tdx_features0 & TDX_FEATURES0_NO_RBP_MOD)) {
+		pr_err("NO_RBP_MOD feature is not supported\n");
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+static void print_tdx_version(void)
+{
+	u64 major, minor, update, internal;
+	int ret;
+
+	ret = read_sys_metadata_field(MD_FIELD_ID_MAJOR_VERSION, &major);
+	if (ret)
+		return;
+
+	ret = read_sys_metadata_field(MD_FIELD_ID_MINOR_VERSION, &minor);
+	if (ret)
+		return;
+
+	ret = read_sys_metadata_field(MD_FIELD_ID_UPDATE_VERSION, &update);
+	if (ret)
+		return;
+
+	ret = read_sys_metadata_field(MD_FIELD_ID_INTERNAL_VERSION, &internal);
+	if (ret)
+		return;
+
+	pr_info("module verson: %u.%u.%u.%u\n",
+			(u16)major, (u16)minor, (u16)update, (u16)internal);
+}
+
 static int init_tdx_module(void)
 {
 	struct tdx_tdmr_sysinfo tdmr_sysinfo;
 	int ret;
 
+	/* Make sure TDX module meets kernel's expectation */
+	ret = check_module_compatibility();
+	if (ret)
+		goto out;
+
 	/*
 	 * To keep things simple, assume that all TDX-protected memory
 	 * will come from the page allocator.  Make sure all pages in the
@@ -1155,6 +1204,10 @@ static int init_tdx_module(void)
 	 * Lock out memory hotplug code while building it.
 	 */
 	put_online_mems();
+
+out:
+	print_tdx_version();
+
 	return ret;
 
 err_reset_pamts:
diff --git a/arch/x86/virt/vmx/tdx/tdx.h b/arch/x86/virt/vmx/tdx/tdx.h
index b701f69485d3..13ad52352e35 100644
--- a/arch/x86/virt/vmx/tdx/tdx.h
+++ b/arch/x86/virt/vmx/tdx/tdx.h
@@ -37,6 +37,14 @@
 #define MD_FIELD_ID_PAMT_2M_ENTRY_SIZE		0x9100000100000011ULL
 #define MD_FIELD_ID_PAMT_1G_ENTRY_SIZE		0x9100000100000012ULL
 
+#define MD_FIELD_ID_TDX_FEATURES0		0x0A00000300000008ULL
+#define TDX_FEATURES0_NO_RBP_MOD		BIT_ULL(18)
+
+#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
+
 /*
  * Sub-field definition of metadata field ID.
  *

Re: [PATCH] x86/kvm/tdx: Save %rbp in TDX_MODULE_CALL

[Jürgen Groß]

On 23.05.24 14:26, Huang, Kai wrote:
> On Thu, 2024-05-23 at 10:30 +0000, Huang, Kai wrote:
>> On Thu, 2024-05-23 at 07:56 +0200, Jürgen Groß wrote:
>>> On 20.05.24 13:54, Huang, Kai wrote:
>>>> On Fri, 2024-05-17 at 09:48 -0700, Dave Hansen wrote:
>>>>> On 5/17/24 08:58, Juergen Gross wrote:
>>>>>> On 17.05.24 17:52, Dave Hansen wrote:
>>>>> ..
>>>>>>> Once we have the specific TDX module version, we can go ask the folks
>>>>>>> who write it if there were any RBP clobbering bugs.
>>>>>>
>>>>>> Okay, how to get the TDX module version?
>>>>>
>>>>> You need something like this:
>>>>>
>>>>>> https://lore.kernel.org/all/20231012134136.1310650-1-yi.sun@intel.com/
>>>>
>>>> This one prints TDX version info in the TDX guest, but not host.
>>>>
>>>> The attached diff prints the TDX version (something like below) during
>>>> module initialization, and should meet Juergen's needs for temporary use:
>>>>
>>>> [  113.543538] virt/tdx: module verson: major 1, minor 5, internal 0
>>>>
>>>>>
>>>>> .. and yeah, this needs to be upstream.
>>>>>
>>>>
>>>>   From this thread I think it makes sense to add code to the TDX host code
>>>> to print the TDX version during module initialization.  I'll start to work
>>>> on this.
>>>>
>>>> One thing is from the spec TDX has "4 versions": major, minor, update,
>>>> internal.  They are all 16-bit, and the overall version can be written in:
>>>>
>>>> 	<Major>.<Minor>.<Update>.<Internal>, e.g., 1.5.05.01
>>>>
>>>> (see TDX module 1.5 API spec, section 3.3.2 "TDX Module Version".)
>>>>
>>>> The attached diff only prints major, minor and internal, but leaves the
>>>> update out because I believe it is for module runtime update (yet to
>>>> confirm).
>>>>
>>>> Given there are 4 versions, I think it makes sense to implement reading
>>>> them based on this patchset ...
>>>>
>>>> https://lore.kernel.org/kvm/6940c326-bfca-4c67-badf-ab5c086bf492@intel.com/T/
>>>>
>>>> ... which extends the global metadata reading code to support any
>>>> arbitrary struct and all element sizes (although all 4 versions are 16-
>>>> bit)?
>>>
>>> With that I got:
>>>
>>> [   29.328484] virt/tdx: module verson: major 1, minor 5, internal 0
>>>
>>>
>>
>> Let me check TDX module guys on this and get back to you.
> 
> Hi Jurgen,
> 
> I was told the module starting with "1.5.06." has NO_RBP_MOD support.
> 
> And I think I was wrong about the <update> part of the version, and we
> need that to determine the third part of the module version.
> 
> I was also told the 1.5.06 module hasn't been released to public yet, so I
> guess your module doesn't support it.
> 
> I did another patch (attached) to check NO_RBP_MOD and reject module
> initialization if it is not supported, and print out module version:
> 
> [  146.566641] virt/tdx: NO_RBP_MOD feature is not supported
> [  146.572797] virt/tdx: module verson: 1.5.0.0
> [  146.577731] virt/tdx: module initialization failed (-22)
> 
> You can have another try to verify at your side, if that helps.
> 

[   29.362806] virt/tdx: 4071192 KB allocated for PAMT
[   29.362828] virt/tdx: module verson: 1.5.1.0
[   29.362830] virt/tdx: module initialized


Juergen

Re: [PATCH] x86/kvm/tdx: Save %rbp in TDX_MODULE_CALL

[Huang, Kai]

On 24/05/2024 12:43 am, Jürgen Groß wrote:
> On 23.05.24 14:26, Huang, Kai wrote:
>> On Thu, 2024-05-23 at 10:30 +0000, Huang, Kai wrote:
>>> On Thu, 2024-05-23 at 07:56 +0200, Jürgen Groß wrote:
>>>> On 20.05.24 13:54, Huang, Kai wrote:
>>>>> On Fri, 2024-05-17 at 09:48 -0700, Dave Hansen wrote:
>>>>>> On 5/17/24 08:58, Juergen Gross wrote:
>>>>>>> On 17.05.24 17:52, Dave Hansen wrote:
>>>>>> ..
>>>>>>>> Once we have the specific TDX module version, we can go ask the 
>>>>>>>> folks
>>>>>>>> who write it if there were any RBP clobbering bugs.
>>>>>>>
>>>>>>> Okay, how to get the TDX module version?
>>>>>>
>>>>>> You need something like this:
>>>>>>
>>>>>>> https://lore.kernel.org/all/20231012134136.1310650-1-yi.sun@intel.com/
>>>>>
>>>>> This one prints TDX version info in the TDX guest, but not host.
>>>>>
>>>>> The attached diff prints the TDX version (something like below) during
>>>>> module initialization, and should meet Juergen's needs for 
>>>>> temporary use:
>>>>>
>>>>> [  113.543538] virt/tdx: module verson: major 1, minor 5, internal 0
>>>>>
>>>>>>
>>>>>> .. and yeah, this needs to be upstream.
>>>>>>
>>>>>
>>>>>   From this thread I think it makes sense to add code to the TDX 
>>>>> host code
>>>>> to print the TDX version during module initialization.  I'll start 
>>>>> to work
>>>>> on this.
>>>>>
>>>>> One thing is from the spec TDX has "4 versions": major, minor, update,
>>>>> internal.  They are all 16-bit, and the overall version can be 
>>>>> written in:
>>>>>
>>>>>     <Major>.<Minor>.<Update>.<Internal>, e.g., 1.5.05.01
>>>>>
>>>>> (see TDX module 1.5 API spec, section 3.3.2 "TDX Module Version".)
>>>>>
>>>>> The attached diff only prints major, minor and internal, but leaves 
>>>>> the
>>>>> update out because I believe it is for module runtime update (yet to
>>>>> confirm).
>>>>>
>>>>> Given there are 4 versions, I think it makes sense to implement 
>>>>> reading
>>>>> them based on this patchset ...
>>>>>
>>>>> https://lore.kernel.org/kvm/6940c326-bfca-4c67-badf-ab5c086bf492@intel.com/T/
>>>>>
>>>>> ... which extends the global metadata reading code to support any
>>>>> arbitrary struct and all element sizes (although all 4 versions are 
>>>>> 16-
>>>>> bit)?
>>>>
>>>> With that I got:
>>>>
>>>> [   29.328484] virt/tdx: module verson: major 1, minor 5, internal 0
>>>>
>>>>
>>>
>>> Let me check TDX module guys on this and get back to you.
>>
>> Hi Jurgen,
>>
>> I was told the module starting with "1.5.06." has NO_RBP_MOD support.
>>
>> And I think I was wrong about the <update> part of the version, and we
>> need that to determine the third part of the module version.
>>
>> I was also told the 1.5.06 module hasn't been released to public yet, 
>> so I
>> guess your module doesn't support it.
>>
>> I did another patch (attached) to check NO_RBP_MOD and reject module
>> initialization if it is not supported, and print out module version:
>>
>> [  146.566641] virt/tdx: NO_RBP_MOD feature is not supported
>> [  146.572797] virt/tdx: module verson: 1.5.0.0
>> [  146.577731] virt/tdx: module initialization failed (-22)
>>
>> You can have another try to verify at your side, if that helps.
>>
> 
> [   29.362806] virt/tdx: 4071192 KB allocated for PAMT
> [   29.362828] virt/tdx: module verson: 1.5.1.0
> [   29.362830] virt/tdx: module initialized

Seems your module supports NO_RBP_MOD.

This feature is per-VM and also requires to be explicitly opt-in when 
creating the guest.  Could you check in your code whether the 
setup_tdparams() function has below code?

	td_params->exec_controls = TDX_CONTROL_FLAG_NO_RBP_MOD;

Re: [PATCH] x86/kvm/tdx: Save %rbp in TDX_MODULE_CALL

[Huang, Kai]

On 24/05/2024 10:34 am, Huang, Kai wrote:
> 
> 
> On 24/05/2024 12:43 am, Jürgen Groß wrote:
>> On 23.05.24 14:26, Huang, Kai wrote:
>>> On Thu, 2024-05-23 at 10:30 +0000, Huang, Kai wrote:
>>>> On Thu, 2024-05-23 at 07:56 +0200, Jürgen Groß wrote:
>>>>> On 20.05.24 13:54, Huang, Kai wrote:
>>>>>> On Fri, 2024-05-17 at 09:48 -0700, Dave Hansen wrote:
>>>>>>> On 5/17/24 08:58, Juergen Gross wrote:
>>>>>>>> On 17.05.24 17:52, Dave Hansen wrote:
>>>>>>> ..
>>>>>>>>> Once we have the specific TDX module version, we can go ask the 
>>>>>>>>> folks
>>>>>>>>> who write it if there were any RBP clobbering bugs.
>>>>>>>>
>>>>>>>> Okay, how to get the TDX module version?
>>>>>>>
>>>>>>> You need something like this:
>>>>>>>
>>>>>>>> https://lore.kernel.org/all/20231012134136.1310650-1-yi.sun@intel.com/
>>>>>>
>>>>>> This one prints TDX version info in the TDX guest, but not host.
>>>>>>
>>>>>> The attached diff prints the TDX version (something like below) 
>>>>>> during
>>>>>> module initialization, and should meet Juergen's needs for 
>>>>>> temporary use:
>>>>>>
>>>>>> [  113.543538] virt/tdx: module verson: major 1, minor 5, internal 0
>>>>>>
>>>>>>>
>>>>>>> .. and yeah, this needs to be upstream.
>>>>>>>
>>>>>>
>>>>>>   From this thread I think it makes sense to add code to the TDX 
>>>>>> host code
>>>>>> to print the TDX version during module initialization.  I'll start 
>>>>>> to work
>>>>>> on this.
>>>>>>
>>>>>> One thing is from the spec TDX has "4 versions": major, minor, 
>>>>>> update,
>>>>>> internal.  They are all 16-bit, and the overall version can be 
>>>>>> written in:
>>>>>>
>>>>>>     <Major>.<Minor>.<Update>.<Internal>, e.g., 1.5.05.01
>>>>>>
>>>>>> (see TDX module 1.5 API spec, section 3.3.2 "TDX Module Version".)
>>>>>>
>>>>>> The attached diff only prints major, minor and internal, but 
>>>>>> leaves the
>>>>>> update out because I believe it is for module runtime update (yet to
>>>>>> confirm).
>>>>>>
>>>>>> Given there are 4 versions, I think it makes sense to implement 
>>>>>> reading
>>>>>> them based on this patchset ...
>>>>>>
>>>>>> https://lore.kernel.org/kvm/6940c326-bfca-4c67-badf-ab5c086bf492@intel.com/T/
>>>>>>
>>>>>> ... which extends the global metadata reading code to support any
>>>>>> arbitrary struct and all element sizes (although all 4 versions 
>>>>>> are 16-
>>>>>> bit)?
>>>>>
>>>>> With that I got:
>>>>>
>>>>> [   29.328484] virt/tdx: module verson: major 1, minor 5, internal 0
>>>>>
>>>>>
>>>>
>>>> Let me check TDX module guys on this and get back to you.
>>>
>>> Hi Jurgen,
>>>
>>> I was told the module starting with "1.5.06." has NO_RBP_MOD support.
>>>
>>> And I think I was wrong about the <update> part of the version, and we
>>> need that to determine the third part of the module version.
>>>
>>> I was also told the 1.5.06 module hasn't been released to public yet, 
>>> so I
>>> guess your module doesn't support it.
>>>
>>> I did another patch (attached) to check NO_RBP_MOD and reject module
>>> initialization if it is not supported, and print out module version:
>>>
>>> [  146.566641] virt/tdx: NO_RBP_MOD feature is not supported
>>> [  146.572797] virt/tdx: module verson: 1.5.0.0
>>> [  146.577731] virt/tdx: module initialization failed (-22)
>>>
>>> You can have another try to verify at your side, if that helps.
>>>
>>
>> [   29.362806] virt/tdx: 4071192 KB allocated for PAMT
>> [   29.362828] virt/tdx: module verson: 1.5.1.0
>> [   29.362830] virt/tdx: module initialized
> 
> Seems your module supports NO_RBP_MOD.
> 
> This feature is per-VM and also requires to be explicitly opt-in when 
> creating the guest.  Could you check in your code whether the 
> setup_tdparams() function has below code?
> 
>      td_params->exec_controls = TDX_CONTROL_FLAG_NO_RBP_MOD;
> 

Oh from another thread I saw you mentioned you have the above code 
enabled.  So from host's perspective the TD should have enabled this 
feature.

It's possible it is a TDX module bug if you are not able to see this 
flag in the guest using the way Kirill replied.

Re: [PATCH] x86/kvm/tdx: Save %rbp in TDX_MODULE_CALL

[Jürgen Groß]

On 24.05.24 00:34, Huang, Kai wrote:
> 
> 
> On 24/05/2024 12:43 am, Jürgen Groß wrote:
>> On 23.05.24 14:26, Huang, Kai wrote:
>>> On Thu, 2024-05-23 at 10:30 +0000, Huang, Kai wrote:
>>>> On Thu, 2024-05-23 at 07:56 +0200, Jürgen Groß wrote:
>>>>> On 20.05.24 13:54, Huang, Kai wrote:
>>>>>> On Fri, 2024-05-17 at 09:48 -0700, Dave Hansen wrote:
>>>>>>> On 5/17/24 08:58, Juergen Gross wrote:
>>>>>>>> On 17.05.24 17:52, Dave Hansen wrote:
>>>>>>> ..
>>>>>>>>> Once we have the specific TDX module version, we can go ask the folks
>>>>>>>>> who write it if there were any RBP clobbering bugs.
>>>>>>>>
>>>>>>>> Okay, how to get the TDX module version?
>>>>>>>
>>>>>>> You need something like this:
>>>>>>>
>>>>>>>> https://lore.kernel.org/all/20231012134136.1310650-1-yi.sun@intel.com/
>>>>>>
>>>>>> This one prints TDX version info in the TDX guest, but not host.
>>>>>>
>>>>>> The attached diff prints the TDX version (something like below) during
>>>>>> module initialization, and should meet Juergen's needs for temporary use:
>>>>>>
>>>>>> [  113.543538] virt/tdx: module verson: major 1, minor 5, internal 0
>>>>>>
>>>>>>>
>>>>>>> .. and yeah, this needs to be upstream.
>>>>>>>
>>>>>>
>>>>>>   From this thread I think it makes sense to add code to the TDX host code
>>>>>> to print the TDX version during module initialization.  I'll start to work
>>>>>> on this.
>>>>>>
>>>>>> One thing is from the spec TDX has "4 versions": major, minor, update,
>>>>>> internal.  They are all 16-bit, and the overall version can be written in:
>>>>>>
>>>>>>     <Major>.<Minor>.<Update>.<Internal>, e.g., 1.5.05.01
>>>>>>
>>>>>> (see TDX module 1.5 API spec, section 3.3.2 "TDX Module Version".)
>>>>>>
>>>>>> The attached diff only prints major, minor and internal, but leaves the
>>>>>> update out because I believe it is for module runtime update (yet to
>>>>>> confirm).
>>>>>>
>>>>>> Given there are 4 versions, I think it makes sense to implement reading
>>>>>> them based on this patchset ...
>>>>>>
>>>>>> https://lore.kernel.org/kvm/6940c326-bfca-4c67-badf-ab5c086bf492@intel.com/T/
>>>>>>
>>>>>> ... which extends the global metadata reading code to support any
>>>>>> arbitrary struct and all element sizes (although all 4 versions are 16-
>>>>>> bit)?
>>>>>
>>>>> With that I got:
>>>>>
>>>>> [   29.328484] virt/tdx: module verson: major 1, minor 5, internal 0
>>>>>
>>>>>
>>>>
>>>> Let me check TDX module guys on this and get back to you.
>>>
>>> Hi Jurgen,
>>>
>>> I was told the module starting with "1.5.06." has NO_RBP_MOD support.
>>>
>>> And I think I was wrong about the <update> part of the version, and we
>>> need that to determine the third part of the module version.
>>>
>>> I was also told the 1.5.06 module hasn't been released to public yet, so I
>>> guess your module doesn't support it.
>>>
>>> I did another patch (attached) to check NO_RBP_MOD and reject module
>>> initialization if it is not supported, and print out module version:
>>>
>>> [  146.566641] virt/tdx: NO_RBP_MOD feature is not supported
>>> [  146.572797] virt/tdx: module verson: 1.5.0.0
>>> [  146.577731] virt/tdx: module initialization failed (-22)
>>>
>>> You can have another try to verify at your side, if that helps.
>>>
>>
>> [   29.362806] virt/tdx: 4071192 KB allocated for PAMT
>> [   29.362828] virt/tdx: module verson: 1.5.1.0
>> [   29.362830] virt/tdx: module initialized
> 
> Seems your module supports NO_RBP_MOD.
> 
> This feature is per-VM and also requires to be explicitly opt-in when creating 
> the guest.  Could you check in your code whether the setup_tdparams() function 
> has below code?
> 
>      td_params->exec_controls = TDX_CONTROL_FLAG_NO_RBP_MOD;

I can confirm that it does have that.


Juergen

Re: [PATCH] x86/kvm/tdx: Save %rbp in TDX_MODULE_CALL

[Sean Christopherson]

On Fri, May 17, 2024, Kirill A. Shutemov wrote:
> On Fri, May 17, 2024 at 02:14:50PM +0200, Juergen Gross wrote:
> > While testing TDX host support patches, a crash of the host has been
> > observed a few instructions after doing a seamcall. Reason was a
> > clobbered %rbp (set to 0), which occurred in spite of the TDX module
> > offering the feature NOT to modify %rbp across TDX module calls.
> > 
> > In order not having to build the host kernel with CONFIG_FRAME_POINTER,
> > save %rbp across a seamcall/tdcall.
> 
> There's a feature in TDX module 1.5 that prevents RBP modification across
> TDH.VP.ENTER SEAMCALL. See NO_RBP_MOD in TDX Module 1.5 ABI spec.

LOL, "feature".  How was clobbering RBP not treated as a bug?  I'm party joking,
but also quite serious.  Unless I'm missing something, the guest ABI changes
based on whether or not NO_RBP_MOD is enabled, as a TDVMCALL that was previously
valid would now fail if the guest attempts to expose RBP to the host.

The whole point of Intel defining a guest-host ABI is to allow interoperability
between hypervisors and guests.  Allowing the hypervisor to arbitrarily change the
ABI is asinine.

> I think it has to be enabled for all TDs and TDX modules that don't
> support it need to be rejected.

Yes, because as above, IIUC it's a breaking change for the guest ABI.

Re: [PATCH] x86/kvm/tdx: Save %rbp in TDX_MODULE_CALL

[Dave Hansen]

On 5/17/24 09:12, Sean Christopherson wrote:
>> There's a feature in TDX module 1.5 that prevents RBP modification across
>> TDH.VP.ENTER SEAMCALL. See NO_RBP_MOD in TDX Module 1.5 ABI spec.
> LOL, "feature".  How was clobbering RBP not treated as a bug?  I'm party joking,
> but also quite serious. 

I'm on the same page.  It would have been far simpler for all involved
to retroactively say that modifying RBP is against the rules and any
module that does it is buggy. Get a new module if yours is buggy.

I _believe_ the intent was to support guest/host combinations that used
RBP for whatever reason.  But I'm not sure such a combination exists or
ever existed in practice.

Re: [PATCH] x86/kvm/tdx: Save %rbp in TDX_MODULE_CALL

[Kirill A. Shutemov]

On Fri, May 17, 2024 at 09:34:56AM -0700, Dave Hansen wrote:
> On 5/17/24 09:12, Sean Christopherson wrote:
> >> There's a feature in TDX module 1.5 that prevents RBP modification across
> >> TDH.VP.ENTER SEAMCALL. See NO_RBP_MOD in TDX Module 1.5 ABI spec.
> > LOL, "feature".  How was clobbering RBP not treated as a bug?  I'm party joking,
> > but also quite serious. 
> 
> I'm on the same page.  It would have been far simpler for all involved
> to retroactively say that modifying RBP is against the rules and any
> module that does it is buggy. Get a new module if yours is buggy.
> 
> I _believe_ the intent was to support guest/host combinations that used
> RBP for whatever reason.  But I'm not sure such a combination exists or
> ever existed in practice.

There's a bug in EDK2. It specifies RBP in mask of registers to pass to
VMM. NO_RBP_MOD breaks it :/

-- 
  Kiryl Shutsemau / Kirill A. Shutemov