Re: [PATCH v7 04/20] x86/virt/tdx: Add skeleton to initialize TDX on demand

[Huang, Kai]

On Wed, 2022-11-23 at 12:05 +0100, Thomas Gleixner wrote:
> Kai!
> 
> On Wed, Nov 23 2022 at 00:30, Kai Huang wrote:
> > On Tue, 2022-11-22 at 21:03 +0100, Thomas Gleixner wrote:
> > > Clearly that's nowhere spelled out in the documentation, but I don't
> > > buy the 'architecturaly required' argument not at all. It's an
> > > implementation detail of the TDX module.
> > 
> > I agree on hardware level there shouldn't be such requirement (not 100% sure
> > though), but I guess from kernel's perspective, "the implementation detail of
> > the TDX module" is sort of "architectural requirement"
> 
> Sure, but then it needs to be clearly documented so.
> 
> > -- at least Intel arch guys think so I guess.
> 
> Intel "arch" guys? You might look up the meanings of "arch" in a
> dictionary. LKML is not twatter.
> 
> > > Technically there is IMO ZERO requirement to do so.
> > > 
> > >  1) The TDX module is global
> > > 
> > >  2) Seam-root and Seam-non-root operation are strictly a LP property.
> > > 
> > >     The only architectural prerequisite for using Seam on a LP is that
> > >     obviously the encryption/decryption mechanics have been initialized
> > >     on the package to which the LP belongs.
> > > 
> > > I can see why it might be complicated to add/remove an LP after
> > > initialization fact, but technically it should be possible.
> > 
> > "kernel soft offline" actually isn't an issue.  We can bring down a logical cpu
> > after it gets initialized and then bring it up again.
> 
> That's the whole point where this discussion started: _AFTER_ it gets
> initialized.
> 
> Which means that, e.g. adding "nosmt" to the kernel command line will
> make TDX fail hard because at the point where TDX is initialized the
> hyperthreads are not 'soft' online and cannot respond to anything, but
> the BIOS already accounted them.
> 
> This is just wrong as we all know that "nosmt" is sadly one of the
> obvious counter measures for the never ending flood of speculation
> issues.

Agree.  As said in my other replies, I'll follow up with TDX module guys on
this.

> 
> > Only add/removal of physical cpu will cause problem: 
> 
> You wish. 
> 
> > TDX MCHECK verifies all boot-time present cpus to make sure they are TDX-
> > compatible before it enables TDX in hardware.  MCHECK cannot run on hot-added
> > CPU, so TDX cannot support physical CPU hotplug.
> 
> TDX can rightfully impose the limitation that it only executes on CPUs,
> which are known at boot/initialization time, and only utilizes "known"
> memory. That's it, but that does not enforce to prevent physical hotplug
> in general.

Adding physical CPUs should have no problem I guess, they just cannot run TDX. 
TDX architecture doesn't expect they can run TDX code anyway.

But would physical removal of boot-time present CPU cause problem? TDX MCHECK
checks/records boot-time present CPUs.  If a CPU is removed and then a new one
is added, then TDX still treats it is TDX-compatible, but it may actually not.

So if this happens, from functionality's point of view, it can break.  I think
TDX still wants to guarantee TDX code can work correctly on "TDX recorded" CPUs.

Also, I am not sure whether there's any security issue if a malicious kernel
tries to run TDX code on such removed-then-added CPU.

This seems a TDX architecture problem rather than kernel policy issue.

> 
> > We tried to get it clarified in the specification, and below is what TDX/module
> > arch guys agreed to put to the TDX module spec (just checked it's not in latest
> > public spec yet, but they said it will be in next release):
> > 
> > "
> > 4.1.3.2.  CPU Configuration
> > 
> > During platform boot, MCHECK verifies all logical CPUs to ensure they
> > meet TDX’s
> 
> That MCHECK falls into the category of security voodoo.
> 
> It needs to verify _ALL_ logical CPUs to ensure that Intel did not put
> different models and steppings into a package or what?

I am guessing so.

> 
> > security and certain functionality requirements, and MCHECK passes the following
> > CPU configuration information to the NP-SEAMLDR, P-SEAMLDR and the TDX Module:
> > 
> > ·         Total number of logical processors in the platform.
> 
> You surely need MCHECK for this
> 
> > ·         Total number of installed packages in the platform.
> 
> and for this...
> 
> > ·         A table of per-package CPU family, model and stepping etc.
> > identification, as enumerated by CPUID(1).EAX.
> > The above information is static and does not change after platform boot and
> > MCHECK run.
> > 
> > Note:     TDX doesn’t support adding or removing CPUs from TDX security
> > perimeter, as checked my MCHECK.
> 
> More security voodoo. The TDX security perimeter has nothing to do with
> adding or removing CPUs on a system. If that'd be true then TDX is a
> complete fail.

No argument here.

> > BIOS should prevent CPUs from being hot-added or hot-removed after
> > platform boots.
> 
> If the BIOS does not prevent it, then TDX and the Seam module will not
> even notice unless the OS tries to invoke seamcall() on a newly plugged
> CPU.
> 
> A newly added CPU and newly added memory should not have any impact on
> the TDX integrity of the already running system. If they have then
> again, TDX is broken by design.

No argument here either.

> 
> > The TDX module performs additional checks of the CPU’s configuration and
> > supported features, by reading MSRs and CPUID information as described in the
> > following sections.
> 
> to ensure that the MCHECK generated table is still valid at the point
> where TDX is initialized? 

I think it is trying to say:

MCHECK doesn't do all the verifications. Some verfications are deferred to the
TDX module to check when it gets initialized.

> 
> That said, this documentation is at least better than the existing void,
> but that does not make it technically correct.
> 
> Thanks,
> 
>         tglx