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 <[email protected]>
---
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
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
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
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
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
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
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.
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. ;)
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
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.
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
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.
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.
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/[email protected]/
.. and yeah, this needs to be upstream.
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
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
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/[email protected]/
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/[email protected]/T/
... which extends the global metadata reading code to support any
arbitrary struct and all element sizes (although all 4 versions are 16-
bit)?
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/[email protected]/
>
> 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/[email protected]/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
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/[email protected]/
> >
> > 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/[email protected]/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.
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/[email protected]/
> > >
> > > 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/[email protected]/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.
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/[email protected]/
>>>>
>>>> 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/[email protected]/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
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/[email protected]/
>>>>>
>>>>> 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/[email protected]/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;
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/[email protected]/
>>>>>>
>>>>>> 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/[email protected]/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.
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/[email protected]/
>>>>>>
>>>>>> 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/[email protected]/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