Change in coreboot[master]: security/intel/stm: Check for processor STM support

[Name of user not set (Code Review)]

[e-mail] has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/38836 )

Change subject: security/intel/stm: Check for processor STM support
......................................................................

security/intel/stm: Check for processor STM support

This check ensures that the current processor supports a STM.
Normally, any Intel x86 processor that has VTX also supports
an STM and this check should fail only in the rare case that STM
support has been disabled for a processor.

Signed-off-by: Eugene D. Myers 
Change-Id: I518bb2aa1bdec94b5b6d5e991d7575257f3dc6e9
---
M src/include/cpu/x86/msr.h
M src/security/intel/stm/StmPlatformSmm.c
2 files changed, 19 insertions(+), 7 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/36/38836/1

diff --git a/src/include/cpu/x86/msr.h b/src/include/cpu/x86/msr.h
index 49abd41..c3764b2 100644
--- a/src/include/cpu/x86/msr.h
+++ b/src/include/cpu/x86/msr.h
@@ -74,6 +74,7 @@
 #define  MCA_STATUS_LO_ERRCODE_EXT_MASK	(0x3f << MCA_STATUS_LO_ERRCODE_EXT_SH)
 #define  MCA_STATUS_LO_ERRCODE_MASK	(0xffff << 0)
 #define IA32_VMX_BASIC_MSR              0x480
+#define  DUAL_MONITOR_TREATMENT_HI      (1 << 17)
 #define IA32_VMX_MISC_MSR               0x485
 #define MC0_ADDR			0x402
 #define MC0_MISC			0x403
diff --git a/src/security/intel/stm/StmPlatformSmm.c b/src/security/intel/stm/StmPlatformSmm.c
index d7064b0..69cbcae 100644
--- a/src/security/intel/stm/StmPlatformSmm.c
+++ b/src/security/intel/stm/StmPlatformSmm.c
@@ -157,11 +157,22 @@
 void stm_setup(uintptr_t mseg, int cpu, int num_cpus, uintptr_t smbase,
    		uintptr_t base_smbase, uint32_t offset32)
 {
-	msr_t InitMseg;
-	msr_t MsegChk;
+	msr_t init_mseg;
+	msr_t mseg_chk;
+	msr_t stm_chk;
+
    uintptr_t addr_calc;  // used to calculate the stm resource heap area

printk(BIOS_DEBUG, "STM: set up for cpu %d/%d\n", cpu, num_cpus);
+
+	stm_chk = rdmsr(IA32_VMX_BASIC_MSR);
+
+	// Does this processor support an STM?
+	if ((stm_chk.hi & DUAL_MONITOR_TREATMENT_HI) != DUAL_MONITOR_TREATMENT_HI) {
+		printk(BIOS_DEBUG, "STM: not supported on cpu: %d\n", cpu);
+		return;
+	}
+
    if (cpu == 0) {

// need to create the BIOS resource list once
@@ -183,15 +194,15 @@

if (stm_load_status == 0) {
    	// enable STM for this cpu
-		InitMseg.lo = mseg | IA32_SMM_MONITOR_VALID;
-		InitMseg.hi = 0;
+		init_mseg.lo = mseg | IA32_SMM_MONITOR_VALID;
+		init_mseg.hi = 0;

-		wrmsr(IA32_SMM_MONITOR_CTL_MSR, InitMseg);
+		wrmsr(IA32_SMM_MONITOR_CTL_MSR, init_mseg);

-		MsegChk = rdmsr(IA32_SMM_MONITOR_CTL_MSR);
+		mseg_chk = rdmsr(IA32_SMM_MONITOR_CTL_MSR);

printk(BIOS_DEBUG, "STM: MSEG Initialized (%d) 0x%08x 0x%08x\n",
-			cpu, MsegChk.hi, MsegChk.lo);
+			cpu, mseg_chk.hi, mseg_chk.lo);

// setup the descriptor for this cpu
    	setup_smm_descriptor((void *)smbase, (void *) base_smbase,

[Paul Menzel (Code Review)]

Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38836 )

Change subject: security/intel/stm: Check for processor STM support
......................................................................

Patch Set 1:

(3 comments)

The variables should have been renamed in a separate commit.

https://review.coreboot.org/c/coreboot/+/38836/1//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/38836/1//COMMIT_MSG@10 
PS1, Line 10: VTX
VTx

https://review.coreboot.org/c/coreboot/+/38836/1/src/security/intel/stm/StmP... 
File src/security/intel/stm/StmPlatformSmm.c:

https://review.coreboot.org/c/coreboot/+/38836/1/src/security/intel/stm/StmP... 
PS1, Line 172: cpu: %d
CPU %d

https://review.coreboot.org/c/coreboot/+/38836/1/src/security/intel/stm/StmP... 
PS1, Line 172: BIOS_DEBUG
This should be at least NOTICE or even WARNING.

[Name of user not set (Code Review)]

[e-mail] has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38836 )

Change subject: security/intel/stm: Check for processor STM support
......................................................................

Patch Set 1:

(2 comments)

https://review.coreboot.org/c/coreboot/+/38836/1/src/security/intel/stm/StmP... 
File src/security/intel/stm/StmPlatformSmm.c:

https://review.coreboot.org/c/coreboot/+/38836/1/src/security/intel/stm/StmP... 
PS1, Line 172: cpu: %d
> CPU %d

Done

https://review.coreboot.org/c/coreboot/+/38836/1/src/security/intel/stm/StmP... 
PS1, Line 172: BIOS_DEBUG
> This should be at least NOTICE or even WARNING.

Done

[Name of user not set (Code Review)]

[e-mail] has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38836 )

Change subject: security/intel/stm: Check for processor STM support
......................................................................

Patch Set 1:

(1 comment)

https://review.coreboot.org/c/coreboot/+/38836/1//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/38836/1//COMMIT_MSG@10 
PS1, Line 10: VTX
> VTx

Done

[Name of user not set (Code Review)]

Hello Patrick Rudolph, ron minnich, build bot (Jenkins), Nicolas Reinecke, Patrick Georgi, Martin Roth,

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/38836

to look at the new patch set (#2).

Change subject: security/intel/stm: Check for processor STM support
......................................................................

security/intel/stm: Check for processor STM support

This check ensures that the current processor supports a STM.
Normally, any Intel x86 processor that has VTx also supports
an STM and this check should fail only in the rare case that STM
support has been disabled for a processor.

Signed-off-by: Eugene D. Myers 
Change-Id: I518bb2aa1bdec94b5b6d5e991d7575257f3dc6e9
---
M src/include/cpu/x86/msr.h
M src/security/intel/stm/StmPlatformSmm.c
2 files changed, 12 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/36/38836/2

[Angel Pons (Code Review)]

Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38836 )

Change subject: security/intel/stm: Check for processor STM support
......................................................................

Patch Set 2:

To be more specific, the processor needs to support dual monitor mode.

[Name of user not set (Code Review)]

[e-mail] has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38836 )

Change subject: security/intel/stm: Check for processor STM support
......................................................................

Patch Set 2:
> Patch Set 2:
>
> To be more specific, the processor needs to support dual monitor mode.

Correct - dual monitor mode is a part of VTx and the STM is a hypervisor that runs in SMM dual monitor mode.  I'll fix the commit message to clarify

[Name of user not set (Code Review)]

Hello Patrick Rudolph, ron minnich, build bot (Jenkins), Nicolas Reinecke, Patrick Georgi, Martin Roth,

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/38836

to look at the new patch set (#3).

Change subject: security/intel/stm: Check for processor STM support
......................................................................

security/intel/stm: Check for processor STM support

Check to ensure that dual monitor mode is supported on the
current processor. Dual monitor mode is normally supported on
any Intel x86 processor that has VTx support.  The STM is
a hypervisor that executes in SMM dual monitor mode.  This
check should fail only in the rare case were dual monitor mode
is disabled.  If the check fails, then the STM will not
be initialized by coreboot.

Signed-off-by: Eugene D. Myers 
Change-Id: I518bb2aa1bdec94b5b6d5e991d7575257f3dc6e9
---
M src/include/cpu/x86/msr.h
M src/security/intel/stm/StmPlatformSmm.c
2 files changed, 12 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/36/38836/3

[Angel Pons (Code Review)]

Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38836 )

Change subject: security/intel/stm: Check for processor STM support
......................................................................

Patch Set 3:

(2 comments)

https://review.coreboot.org/c/coreboot/+/38836/3/src/security/intel/stm/StmP... 
File src/security/intel/stm/StmPlatformSmm.c:

https://review.coreboot.org/c/coreboot/+/38836/3/src/security/intel/stm/StmP... 
PS3, Line 162: stm_chk
nit: maybe call this vmx_basic ?

https://review.coreboot.org/c/coreboot/+/38836/3/src/security/intel/stm/StmP... 
PS3, Line 172: BIOS_WARNING
How bad is it when STM has been enabled, but the CPU does not support it? Can the system still boot successfully? If so, can it run without any security issues, or would it result in security weaknesses?

[Name of user not set (Code Review)]

[e-mail] has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38836 )

Change subject: security/intel/stm: Check for processor STM support
......................................................................

Patch Set 3:

(2 comments)

https://review.coreboot.org/c/coreboot/+/38836/3/src/security/intel/stm/StmP... 
File src/security/intel/stm/StmPlatformSmm.c:

https://review.coreboot.org/c/coreboot/+/38836/3/src/security/intel/stm/StmP... 
PS3, Line 162: stm_chk
> nit: maybe call this vmx_basic ?

Done

https://review.coreboot.org/c/coreboot/+/38836/3/src/security/intel/stm/StmP... 
PS3, Line 172: BIOS_WARNING
> How bad is it when STM has been enabled, but the CPU does not support it? Can the system still boot  […]

If the DUAL_MONITOR_TREATMENT flag is off in the IA32_VMX_BASIC_MSR and there is an attempt to access the IA32_MONITOR_CTL_MSR (either WRMSR or RDMSR), a GP fault will be generated, which will keep the system from boot (aka brick).  This check makes sure that this does not happen and give the developer a chance to address the situation (like, why is this bit not set?)
In the rare event that the bit does not get set, that means that the STM will not get setup.  One consequence of this, is that the IA32_SMM_MONITOR_VALID bit does not get set in the IA32_SMM_MONITOR_CTL_MSR.
The IA32_SMM_MONITOR_VALID should be checked by the operating system/hypervisor, etc. before starting the STM.  If the IA32_SMM_MONITOR_VALID is not set and the O/S attempts to start the STM a GP fault will occur.

By checking the IA32_SMM_MONITOR_VALID bit, which means that the STM has not been setup by the firmware, the O/S and/or the system owner can take remedial action since they are expecting an STM to be there.

[Name of user not set (Code Review)]

Hello Patrick Rudolph, ron minnich, build bot (Jenkins), Nicolas Reinecke, Patrick Georgi, Martin Roth,

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/38836

to look at the new patch set (#4).

Change subject: security/intel/stm: Check for processor STM support
......................................................................

security/intel/stm: Check for processor STM support

Check to ensure that dual monitor mode is supported on the
current processor. Dual monitor mode is normally supported on
any Intel x86 processor that has VTx support.  The STM is
a hypervisor that executes in SMM dual monitor mode.  This
check should fail only in the rare case were dual monitor mode
is disabled.  If the check fails, then the STM will not
be initialized by coreboot.

Signed-off-by: Eugene D. Myers 
Change-Id: I518bb2aa1bdec94b5b6d5e991d7575257f3dc6e9
---
M src/include/cpu/x86/msr.h
M src/security/intel/stm/StmPlatformSmm.c
2 files changed, 12 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/36/38836/4

[Nico Huber (Code Review)]

Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38836 )

Change subject: security/intel/stm: Check for processor STM support
......................................................................

Patch Set 5: Code-Review+1

(1 comment)

https://review.coreboot.org/c/coreboot/+/38836/5/src/include/cpu/x86/msr.h 
File src/include/cpu/x86/msr.h:

https://review.coreboot.org/c/coreboot/+/38836/5/src/include/cpu/x86/msr.h@7... 
PS5, Line 77: 17
It's documented as bit 49, so `(49 - 32)` is be preferred, cf. MCA_STATUS_HI_*.

Also, how about naming it VMX_BASIC_HI_DUAL_MONITOR? or at least give it some
VMX_ prefix.

[Name of user not set (Code Review)]

[e-mail] has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38836 )

Change subject: security/intel/stm: Check for processor STM support
......................................................................

Patch Set 5:

(1 comment)

https://review.coreboot.org/c/coreboot/+/38836/5/src/include/cpu/x86/msr.h 
File src/include/cpu/x86/msr.h:

https://review.coreboot.org/c/coreboot/+/38836/5/src/include/cpu/x86/msr.h@7... 
PS5, Line 77: 17
> It's documented as bit 49, so `(49 - 32)` is be preferred, cf. MCA_STATUS_HI_*. […]

Done

[Name of user not set (Code Review)]

Hello Patrick Rudolph, ron minnich, build bot (Jenkins), Nico Huber, Nicolas Reinecke, Patrick Georgi, Martin Roth,

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/38836

to look at the new patch set (#6).

Change subject: security/intel/stm: Check for processor STM support
......................................................................

security/intel/stm: Check for processor STM support

Check to ensure that dual monitor mode is supported on the
current processor. Dual monitor mode is normally supported on
any Intel x86 processor that has VTx support.  The STM is
a hypervisor that executes in SMM dual monitor mode.  This
check should fail only in the rare case were dual monitor mode
is disabled.  If the check fails, then the STM will not
be initialized by coreboot.

Signed-off-by: Eugene D. Myers 
Change-Id: I518bb2aa1bdec94b5b6d5e991d7575257f3dc6e9
---
M src/include/cpu/x86/msr.h
M src/security/intel/stm/StmPlatformSmm.c
2 files changed, 12 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/36/38836/6

[Nico Huber (Code Review)]

Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38836 )

Change subject: security/intel/stm: Check for processor STM support
......................................................................

Patch Set 6: Code-Review+2

[Nico Huber (Code Review)]

Nico Huber has submitted this change. ( https://review.coreboot.org/c/coreboot/+/38836 )

Change subject: security/intel/stm: Check for processor STM support
......................................................................

security/intel/stm: Check for processor STM support

Check to ensure that dual monitor mode is supported on the
current processor. Dual monitor mode is normally supported on
any Intel x86 processor that has VTx support.  The STM is
a hypervisor that executes in SMM dual monitor mode.  This
check should fail only in the rare case were dual monitor mode
is disabled.  If the check fails, then the STM will not
be initialized by coreboot.

Signed-off-by: Eugene D. Myers 
Change-Id: I518bb2aa1bdec94b5b6d5e991d7575257f3dc6e9
Reviewed-on: https://review.coreboot.org/c/coreboot/+/38836
Tested-by: build bot (Jenkins) 
Reviewed-by: Nico Huber 
---
M src/include/cpu/x86/msr.h
M src/security/intel/stm/StmPlatformSmm.c
2 files changed, 12 insertions(+), 0 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  Nico Huber: Looks good to me, approved

diff --git a/src/include/cpu/x86/msr.h b/src/include/cpu/x86/msr.h
index 49abd41..c761bc0 100644
--- a/src/include/cpu/x86/msr.h
+++ b/src/include/cpu/x86/msr.h
@@ -74,6 +74,7 @@
 #define  MCA_STATUS_LO_ERRCODE_EXT_MASK	(0x3f << MCA_STATUS_LO_ERRCODE_EXT_SH)
 #define  MCA_STATUS_LO_ERRCODE_MASK	(0xffff << 0)
 #define IA32_VMX_BASIC_MSR              0x480
+#define  VMX_BASIC_HI_DUAL_MONITOR      (1UL << (49 - 32))
 #define IA32_VMX_MISC_MSR               0x485
 #define MC0_ADDR			0x402
 #define MC0_MISC			0x403
diff --git a/src/security/intel/stm/StmPlatformSmm.c b/src/security/intel/stm/StmPlatformSmm.c
index d7064b0..45db0e0 100644
--- a/src/security/intel/stm/StmPlatformSmm.c
+++ b/src/security/intel/stm/StmPlatformSmm.c
@@ -159,9 +159,20 @@
 {
    msr_t InitMseg;
    msr_t MsegChk;
+	msr_t vmx_basic;
+
    uintptr_t addr_calc;  // used to calculate the stm resource heap area

printk(BIOS_DEBUG, "STM: set up for cpu %d/%d\n", cpu, num_cpus);
+
+	vmx_basic = rdmsr(IA32_VMX_BASIC_MSR);
+
+	// Does this processor support an STM?
+	if ((vmx_basic.hi & VMX_BASIC_HI_DUAL_MONITOR) != VMX_BASIC_HI_DUAL_MONITOR) {
+		printk(BIOS_WARNING, "STM: not supported on CPU %d\n", cpu);
+		return;
+	}
+
    if (cpu == 0) {

// need to create the BIOS resource list once