Change in coreboot[master]: security/intel/txt: Improve MTRR setup for GETSEC[ENTERACCS]
Angel Pons has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/44880 )
Change subject: security/intel/txt: Improve MTRR setup for GETSEC[ENTERACCS] ......................................................................
security/intel/txt: Improve MTRR setup for GETSEC[ENTERACCS]
The BIOS ACM will check that enabled variable MTRRs do not cover more than the ACM's size, rounded up to 4 KiB. If that is not the case, launching the ACM will result in a lovely TXT reset. How boring.
The new algorithm simply performs a reverse bit scan in a loop, and allocates one MTRR for each set bit in the rounded-up size to cache. Before allocating anything, it checks if there are enough variable MTRRs; if not, it will refuse to cache anything. This will result in another TXT reset, initiated by the processor, with error type 5:
Load memory type error in Authenticated Code Execution Area.
This can only happen if the ACM has different caching requirements that the current code does not know about, or something has been compromised. Therefore, causing a TXT reset should be a reasonable enough approach.
Also, disable all MTRRs before clearing the variable MTRRs and only enable them again once they have been set up with the new values.
Tested on Asrock B85M Pro4 with a BIOS ACM whose size is 101504 bytes. Without this patch, launching the ACM would result in a TXT reset. This no longer happens when this patch is appliedd.
Change-Id: I8d411f6450928357544be20250262c2005d1e75d Signed-off-by: Angel Pons th3fanbus@gmail.com --- M src/security/intel/txt/getsec_enteraccs.S 1 file changed, 94 insertions(+), 18 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/80/44880/1
diff --git a/src/security/intel/txt/getsec_enteraccs.S b/src/security/intel/txt/getsec_enteraccs.S index 3135de7..b78ab8a 100644 --- a/src/security/intel/txt/getsec_enteraccs.S +++ b/src/security/intel/txt/getsec_enteraccs.S @@ -107,10 +107,10 @@ orl $(CR0_CD | CR0_NW), %eax movl %eax, %cr0
- /* Disable fixed MTRRs */ + /* Disable all MTRRs */ movl $(MTRR_DEF_TYPE_MSR), %ecx - rdmsr - andl $(~MTRR_DEF_TYPE_FIX_EN), %eax + xorl %eax, %eax + xorl %edx, %edx wrmsr
/* @@ -164,24 +164,100 @@ * Chapter A.1.1 * Intel TXT Software Development Guide (Document: 315168-015) */ - movl $(MTRR_PHYS_BASE(0)), %ecx - movl 12(%ebp), %eax /* %eax = acmbase */ - orl $(6), %eax /* MTRR_TYPE_WB */ - movl $0, %edx - wrmsr
- /* Round acmsize to next power of two. Required for MTRR programming. */ + /* + * Important note: The MTRRs must cache less than a page (4 KiB) + * of unused memory after the BIOS ACM. Failure to do so will + * result in a TXT reset with Class Code 5, Major Error Code 2. + */ + + /* Determine size of AC module */ + movl 12(%ebp), %eax /* %eax = acmbase */ movl $1, %ebx - movl 16(%ebp), %ecx /* %ebx = acmsize */ - dec %ecx - bsr %ecx, %ecx /* find MSB */ - inc %ecx - shl %cl, %ebx - movl $(MTRR_PHYS_MASK(0)), %ecx - xorl %eax, %eax - subl %ebx, %eax /* %eax = 4GIB - log2_ceil(ACM SIZE) */ - orl $((1 << 11)), %eax /* MTRR_PHYS_MASK_VALID */ + movl 16(%ebp), %ebx /* %ebx = acmsize */ + + /* Round up to page size */ + addl $(0xfff), %ebx + andl $(~0xfff), %ebx /* Aligned to a page (4 KiB) */ + + /* Use XMM to store local variables */ + movd %eax, %xmm0 /* XMM0: Base address of next MTRR */ + movd %ebx, %xmm1 /* XMM1: Remaining size to cache */ + + /* Get the number of variable MTRRs */ + movl $(MTRR_CAP_MSR), %ecx + rdmsr + andl $(0xff), %eax + + /* Initialize ECX */ + movl $(MTRR_PHYS_BASE(0)), %ecx + + /* Check if there are enough variable MTRRs to cache this size */ + popcnt %ebx, %edx + cmp %eax, %edx + + jbe cond_allocate_var_mtrrs + + /* + * There aren't enough MTRRs to handle this size. + * The ACM will be uncached, and will invoke TXT reset. + * + * FIXME: Do proper error handling? + */ + xorl %ebx, %ebx + movd %ebx, %xmm1 + + jmp cond_allocate_var_mtrrs + +body_allocate_var_mtrrs: + + /* Program MTRR base */ + xorl %edx, %edx + movd %xmm0, %eax + orl $(MTRR_TYPE_WRBACK), %eax + wrmsr + incl %ecx /* MTRR_PHYS_MASK */ + + /* Temporarily transfer MSR index to EDX so that CL can be used */ + movl %ecx, %edx + + /* Determine next size to cache */ + bsr %ebx, %ecx + movl $(1), %ebx + shl %cl, %ebx /* Can only use CL here */ + + /* Restore ECX */ + movl %edx, %ecx + + /* Update saved base address */ + addl %ebx, %eax + movd %eax, %xmm0 + + /* Update saved remaining size */ + movd %xmm1, %eax + subl %ebx, %eax + movd %eax, %xmm1 + + /* Program MTRR mask */ movl MTRR_HIGH_MASK, %edx + xorl %eax, %eax + subl %ebx, %eax /* %eax = 4GIB - size to cache */ + orl $((1 << 11)), %eax /* MTRR_PHYS_MASK_VALID */ + wrmsr + incl %ecx /* Next MTRR_PHYS_BASE */ + +cond_allocate_var_mtrrs: + + /* Check if we still need to cache something */ + movd %xmm1, %ebx + andl %ebx, %ebx + + jnz body_allocate_var_mtrrs + + /* Enable variable MTRRs */ + movl $(MTRR_DEF_TYPE_MSR), %ecx + rdmsr + orl $(MTRR_DEF_TYPE_EN), %eax wrmsr
/* Enable cache - GPF# if not done */
Hello Philipp Deppenwiese, Patrick Rudolph, Jonathan Zhang, Arthur Heymans,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/44880
to look at the new patch set (#2).
Change subject: security/intel/txt: Improve MTRR setup for GETSEC[ENTERACCS] ......................................................................
security/intel/txt: Improve MTRR setup for GETSEC[ENTERACCS]
The BIOS ACM will check that enabled variable MTRRs do not cover more than the ACM's size, rounded up to 4 KiB. If that is not the case, launching the ACM will result in a lovely TXT reset. How boring.
The new algorithm simply performs a reverse bit scan in a loop, and allocates one MTRR for each set bit in the rounded-up size to cache. Before allocating anything, it checks if there are enough variable MTRRs; if not, it will refuse to cache anything. This will result in another TXT reset, initiated by the processor, with error type 5:
Load memory type error in Authenticated Code Execution Area.
This can only happen if the ACM has specific caching requirements that the current code does not know about, or something has been compromised. Therefore, causing a TXT reset should be a reasonable enough approach.
Also, disable all MTRRs before clearing the variable MTRRs and only enable them again once they have been set up with the new values.
Tested on Asrock B85M Pro4 with a BIOS ACM whose size is 101504 bytes. Without this patch, launching the ACM would result in a TXT reset. This no longer happens when this patch is appliedd.
Change-Id: I8d411f6450928357544be20250262c2005d1e75d Signed-off-by: Angel Pons th3fanbus@gmail.com --- M src/security/intel/txt/getsec_enteraccs.S 1 file changed, 95 insertions(+), 18 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/80/44880/2
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44880 )
Change subject: security/intel/txt: Improve MTRR setup for GETSEC[ENTERACCS] ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/44880/2/src/security/intel/txt/gets... File src/security/intel/txt/getsec_enteraccs.S:
https://review.coreboot.org/c/coreboot/+/44880/2/src/security/intel/txt/gets... PS2, Line 184: movd %eax, %xmm0 /* XMM0: Base address of next MTRR */ that only works if CONFIG_SSE is set
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44880 )
Change subject: security/intel/txt: Improve MTRR setup for GETSEC[ENTERACCS] ......................................................................
Patch Set 2:
(2 comments)
https://review.coreboot.org/c/coreboot/+/44880/2/src/security/intel/txt/gets... File src/security/intel/txt/getsec_enteraccs.S:
https://review.coreboot.org/c/coreboot/+/44880/2/src/security/intel/txt/gets... PS2, Line 184: movd %eax, %xmm0 /* XMM0: Base address of next MTRR */
that only works if CONFIG_SSE is set
We're already using SSE registers in the code below. In fact, it's why I decided to use SSE registers in the first place.
https://review.coreboot.org/c/coreboot/+/44880/2/src/security/intel/txt/gets... PS2, Line 286: /* Backup stack pointer */ : movd %esp, %xmm0 : movd %ebp, %xmm1 See here
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44880 )
Change subject: security/intel/txt: Improve MTRR setup for GETSEC[ENTERACCS] ......................................................................
Patch Set 8:
Can the MTRR operations moved to C? That would make it easier to review and it would be possible to use existing common code.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44880 )
Change subject: security/intel/txt: Improve MTRR setup for GETSEC[ENTERACCS] ......................................................................
Patch Set 8:
Patch Set 8:
Can the MTRR operations moved to C? That would make it easier to review and it would be possible to use existing common code.
I use the same assembly code to invoke GETSEC in romstage, and I need to tear down CAR before reprogramming the MTRRs. And this is required on LT-CX platforms, since a reset with secrets in memory will block the memory controller until one invokes the SCLEAN function of the BIOS ACM.
So yes, it could be done, but would not work for pre-RAM stages.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44880 )
Change subject: security/intel/txt: Improve MTRR setup for GETSEC[ENTERACCS] ......................................................................
Patch Set 8:
(1 comment)
https://review.coreboot.org/c/coreboot/+/44880/8//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/44880/8//COMMIT_MSG@30 PS8, Line 30: appliedd one `d`
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44880 )
Change subject: security/intel/txt: Improve MTRR setup for GETSEC[ENTERACCS] ......................................................................
Patch Set 9:
(1 comment)
https://review.coreboot.org/c/coreboot/+/44880/2/src/security/intel/txt/gets... File src/security/intel/txt/getsec_enteraccs.S:
https://review.coreboot.org/c/coreboot/+/44880/2/src/security/intel/txt/gets... PS2, Line 184: movd %eax, %xmm0 /* XMM0: Base address of next MTRR */
We're already using SSE registers in the code below. […]
Ack
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44880 )
Change subject: security/intel/txt: Improve MTRR setup for GETSEC[ENTERACCS] ......................................................................
Patch Set 9:
Patch Set 8:
Patch Set 8:
Can the MTRR operations moved to C? That would make it easier to review and it would be possible to use existing common code.
I use the same assembly code to invoke GETSEC in romstage, and I need to tear down CAR before reprogramming the MTRRs. And this is required on LT-CX platforms, since a reset with secrets in memory will block the memory controller until one invokes the SCLEAN function of the BIOS ACM.
So yes, it could be done, but would not work for pre-RAM stages.
Maybe the MTRR computation can be done in C and just passed on to the assembly call? At first sight it might be a good idea to have separate code for romstage/in-CAR calls since you need to be more careful w.r.t. tearing down CAR. e.g. for the romstage version could save the MTRR setup computed in C code in MMX/XMM FPU registers before applying them. The ramstage version can just access that from stack.
Hello Philipp Deppenwiese, build bot (Jenkins), Patrick Rudolph, Jonathan Zhang, Christian Walter, Arthur Heymans,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/44880
to look at the new patch set (#11).
Change subject: security/intel/txt: Improve MTRR setup for GETSEC[ENTERACCS] ......................................................................
security/intel/txt: Improve MTRR setup for GETSEC[ENTERACCS]
The BIOS ACM will check that enabled variable MTRRs do not cover more than the ACM's size, rounded up to 4 KiB. If that is not the case, launching the ACM will result in a lovely TXT reset. How boring.
The new algorithm simply performs a reverse bit scan in a loop, and allocates one MTRR for each set bit in the rounded-up size to cache. Before allocating anything, it checks if there are enough variable MTRRs; if not, it will refuse to cache anything. This will result in another TXT reset, initiated by the processor, with error type 5:
Load memory type error in Authenticated Code Execution Area.
This can only happen if the ACM has specific caching requirements that the current code does not know about, or something has been compromised. Therefore, causing a TXT reset should be a reasonable enough approach.
Also, disable all MTRRs before clearing the variable MTRRs and only enable them again once they have been set up with the new values.
Tested on Asrock B85M Pro4 with a BIOS ACM whose size is 101504 bytes. Without this patch, launching the ACM would result in a TXT reset. This no longer happens when this patch is applied.
Change-Id: I8d411f6450928357544be20250262c2005d1e75d Signed-off-by: Angel Pons th3fanbus@gmail.com --- M src/security/intel/txt/getsec_enteraccs.S 1 file changed, 95 insertions(+), 18 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/80/44880/11
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44880 )
Change subject: security/intel/txt: Improve MTRR setup for GETSEC[ENTERACCS] ......................................................................
Patch Set 11:
(1 comment)
https://review.coreboot.org/c/coreboot/+/44880/8//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/44880/8//COMMIT_MSG@30 PS8, Line 30: appliedd
one `d`
Done
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44880 )
Change subject: security/intel/txt: Improve MTRR setup for GETSEC[ENTERACCS] ......................................................................
Patch Set 11:
(4 comments)
Looking good!
I'd think that passing a MTRR handof struct (maybe even reuse the one from postcar) and setting things up in C code could make our lives a lot easier?
https://review.coreboot.org/c/coreboot/+/44880/11/src/security/intel/txt/get... File src/security/intel/txt/getsec_enteraccs.S:
https://review.coreboot.org/c/coreboot/+/44880/11/src/security/intel/txt/get... PS11, Line 183: Use XMM to store local variables Maybe we should document that after all MTRR's are saved, the rest should be stackless?
https://review.coreboot.org/c/coreboot/+/44880/11/src/security/intel/txt/get... PS11, Line 195: /* Check if there are enough variable MTRRs to cache this size */ : popcnt %ebx, %edx : cmp %eax, %edx I guess some of the checks could be moved to C code before calling assembly? Probably a thing to do later on.
https://review.coreboot.org/c/coreboot/+/44880/11/src/security/intel/txt/get... PS11, Line 243: MTRR_HIGH_MASK Sidenote: This can be fetched dynamically (see CAR setups).
https://review.coreboot.org/c/coreboot/+/44880/11/src/security/intel/txt/get... PS11, Line 246: (1 << 11) just use MTRR_PHYS_MASK_VALID.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44880 )
Change subject: security/intel/txt: Improve MTRR setup for GETSEC[ENTERACCS] ......................................................................
Patch Set 11:
(3 comments)
Patch Set 11:
(4 comments)
Looking good!
I'd think that passing a MTRR handof struct (maybe even reuse the one from postcar) and setting things up in C code could make our lives a lot easier?
It would make things harder for Haswell, though: I need to call the SCLEAN function of the BIOS ACM in early romstage if there are secrets from DRAM. If I don't do it, memory initialization will fail (MRC hangs; I think because the MPLL never starts up). The current, known-working implementation I have uses roughly the same MTRR setup code, but with a different prologue/epilogue.
I'm in process of extracting this romstage SCLEAN launch out of the big 'golden' (works, but is heavy) change I have.
https://review.coreboot.org/c/coreboot/+/44880/11/src/security/intel/txt/get... File src/security/intel/txt/getsec_enteraccs.S:
https://review.coreboot.org/c/coreboot/+/44880/11/src/security/intel/txt/get... PS11, Line 183: Use XMM to store local variables
Maybe we should document that after all MTRR's are saved, the rest should be stackless?
I decided to use XMM registers for this because I saw them being used later on (right before filling in the parameters for the getsec instruction), and decided to use them here too. Plus, I don't have any stack in romstage (I tore it down earlier), and there I use four XMM registers.
https://review.coreboot.org/c/coreboot/+/44880/11/src/security/intel/txt/get... PS11, Line 195: /* Check if there are enough variable MTRRs to cache this size */ : popcnt %ebx, %edx : cmp %eax, %edx
I guess some of the checks could be moved to C code before calling assembly? Probably a thing to do […]
This could be done, yes. It would also avoid having the error path below, since assembly would then not be called. OTOH, I like the simplicity of using `popcnt` to check this 😎
Reminds me that the BIOS ACM needs to be aligned to its size (or the next largest power of 2) and this is checked in C code already.
https://review.coreboot.org/c/coreboot/+/44880/11/src/security/intel/txt/get... PS11, Line 246: (1 << 11)
just use MTRR_PHYS_MASK_VALID.
Good catch. I reused the original code, and missed this.
Hello Philipp Deppenwiese, build bot (Jenkins), Patrick Rudolph, Jonathan Zhang, Christian Walter, Arthur Heymans,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/44880
to look at the new patch set (#12).
Change subject: security/intel/txt: Improve MTRR setup for GETSEC[ENTERACCS] ......................................................................
security/intel/txt: Improve MTRR setup for GETSEC[ENTERACCS]
The BIOS ACM will check that enabled variable MTRRs do not cover more than the ACM's size, rounded up to 4 KiB. If that is not the case, launching the ACM will result in a lovely TXT reset. How boring.
The new algorithm simply performs a reverse bit scan in a loop, and allocates one MTRR for each set bit in the rounded-up size to cache. Before allocating anything, it checks if there are enough variable MTRRs; if not, it will refuse to cache anything. This will result in another TXT reset, initiated by the processor, with error type 5:
Load memory type error in Authenticated Code Execution Area.
This can only happen if the ACM has specific caching requirements that the current code does not know about, or something has been compromised. Therefore, causing a TXT reset should be a reasonable enough approach.
Also, disable all MTRRs before clearing the variable MTRRs and only enable them again once they have been set up with the new values.
Tested on Asrock B85M Pro4 with a BIOS ACM whose size is 101504 bytes. Without this patch, launching the ACM would result in a TXT reset. This no longer happens when this patch is applied.
Change-Id: I8d411f6450928357544be20250262c2005d1e75d Signed-off-by: Angel Pons th3fanbus@gmail.com --- M src/security/intel/txt/getsec_enteraccs.S 1 file changed, 87 insertions(+), 18 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/80/44880/12
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44880 )
Change subject: security/intel/txt: Improve MTRR setup for GETSEC[ENTERACCS] ......................................................................
Patch Set 12:
(3 comments)
https://review.coreboot.org/c/coreboot/+/44880/11/src/security/intel/txt/get... File src/security/intel/txt/getsec_enteraccs.S:
https://review.coreboot.org/c/coreboot/+/44880/11/src/security/intel/txt/get... PS11, Line 183: Use XMM to store local variables
I decided to use XMM registers for this because I saw them being used later on (right before filling […]
I expanded the comment a bit, thoughts?
https://review.coreboot.org/c/coreboot/+/44880/11/src/security/intel/txt/get... PS11, Line 195: /* Check if there are enough variable MTRRs to cache this size */ : popcnt %ebx, %edx : cmp %eax, %edx
This could be done, yes. […]
Done in CB:46422
https://review.coreboot.org/c/coreboot/+/44880/11/src/security/intel/txt/get... PS11, Line 246: (1 << 11)
Good catch. I reused the original code, and missed this.
Done
Arthur Heymans has uploaded a new patch set (#14) to the change originally created by Angel Pons. ( https://review.coreboot.org/c/coreboot/+/44880 )
Change subject: security/intel/txt: Improve MTRR setup for GETSEC[ENTERACCS] ......................................................................
security/intel/txt: Improve MTRR setup for GETSEC[ENTERACCS]
The BIOS ACM will check that enabled variable MTRRs do not cover more than the ACM's size, rounded up to 4 KiB. If that is not the case, launching the ACM will result in a lovely TXT reset. How boring.
The new algorithm simply performs a reverse bit scan in a loop, and allocates one MTRR for each set bit in the rounded-up size to cache. Before allocating anything, it checks if there are enough variable MTRRs; if not, it will refuse to cache anything. This will result in another TXT reset, initiated by the processor, with error type 5:
Load memory type error in Authenticated Code Execution Area.
This can only happen if the ACM has specific caching requirements that the current code does not know about, or something has been compromised. Therefore, causing a TXT reset should be a reasonable enough approach.
Also, disable all MTRRs before clearing the variable MTRRs and only enable them again once they have been set up with the new values.
Tested on Asrock B85M Pro4 with a BIOS ACM whose size is 101504 bytes. Without this patch, launching the ACM would result in a TXT reset. This no longer happens when this patch is appliedd.
Change-Id: I8d411f6450928357544be20250262c2005d1e75d Signed-off-by: Angel Pons th3fanbus@gmail.com --- M src/security/intel/txt/getsec_enteraccs.S 1 file changed, 95 insertions(+), 18 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/80/44880/14
Hello Philipp Deppenwiese, build bot (Jenkins), Patrick Rudolph, Jonathan Zhang, Christian Walter, Arthur Heymans,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/44880
to look at the new patch set (#15).
Change subject: security/intel/txt: Improve MTRR setup for GETSEC[ENTERACCS] ......................................................................
security/intel/txt: Improve MTRR setup for GETSEC[ENTERACCS]
The BIOS ACM will check that enabled variable MTRRs do not cover more than the ACM's size, rounded up to 4 KiB. If that is not the case, launching the ACM will result in a lovely TXT reset. How boring.
The new algorithm simply performs a reverse bit scan in a loop, and allocates one MTRR for each set bit in the rounded-up size to cache. Before allocating anything, it checks if there are enough variable MTRRs; if not, it will refuse to cache anything. This will result in another TXT reset, initiated by the processor, with error type 5:
Load memory type error in Authenticated Code Execution Area.
This can only happen if the ACM has specific caching requirements that the current code does not know about, or something has been compromised. Therefore, causing a TXT reset should be a reasonable enough approach.
Also, disable all MTRRs before clearing the variable MTRRs and only enable them again once they have been set up with the new values.
Tested on Asrock B85M Pro4 with a BIOS ACM whose size is 101504 bytes. Without this patch, launching the ACM would result in a TXT reset. This no longer happens when this patch is applied.
Change-Id: I8d411f6450928357544be20250262c2005d1e75d Signed-off-by: Angel Pons th3fanbus@gmail.com --- M src/security/intel/txt/getsec_enteraccs.S 1 file changed, 87 insertions(+), 18 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/80/44880/15
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44880 )
Change subject: security/intel/txt: Improve MTRR setup for GETSEC[ENTERACCS] ......................................................................
Patch Set 15: Code-Review+2
(2 comments)
LGTM
https://review.coreboot.org/c/coreboot/+/44880/15/src/security/intel/txt/get... File src/security/intel/txt/getsec_enteraccs.S:
https://review.coreboot.org/c/coreboot/+/44880/15/src/security/intel/txt/get... PS15, Line 111: Disable all MTRRs Maybe Also say the default type is UC?
https://review.coreboot.org/c/coreboot/+/44880/15/src/security/intel/txt/get... PS15, Line 218: $(1) nit, $1 looks better, but I don't care.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44880 )
Change subject: security/intel/txt: Improve MTRR setup for GETSEC[ENTERACCS] ......................................................................
Patch Set 15:
(1 comment)
https://review.coreboot.org/c/coreboot/+/44880/15/src/security/intel/txt/get... File src/security/intel/txt/getsec_enteraccs.S:
https://review.coreboot.org/c/coreboot/+/44880/15/src/security/intel/txt/get... PS15, Line 111: Disable all MTRRs
Maybe Also say the default type is UC?
I'd expect that to be quite clear already.
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44880 )
Change subject: security/intel/txt: Improve MTRR setup for GETSEC[ENTERACCS] ......................................................................
Patch Set 15:
(1 comment)
https://review.coreboot.org/c/coreboot/+/44880/15/src/security/intel/txt/get... File src/security/intel/txt/getsec_enteraccs.S:
https://review.coreboot.org/c/coreboot/+/44880/15/src/security/intel/txt/get... PS15, Line 111: Disable all MTRRs
I'd expect that to be quite clear already.
Ack
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44880 )
Change subject: security/intel/txt: Improve MTRR setup for GETSEC[ENTERACCS] ......................................................................
Patch Set 15:
(1 comment)
https://review.coreboot.org/c/coreboot/+/44880/11/src/security/intel/txt/get... File src/security/intel/txt/getsec_enteraccs.S:
https://review.coreboot.org/c/coreboot/+/44880/11/src/security/intel/txt/get... PS11, Line 183: Use XMM to store local variables
I expanded the comment a bit, thoughts?
Ack
Christian Walter has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44880 )
Change subject: security/intel/txt: Improve MTRR setup for GETSEC[ENTERACCS] ......................................................................
Patch Set 15: Code-Review+2
Jonathan Zhang has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44880 )
Change subject: security/intel/txt: Improve MTRR setup for GETSEC[ENTERACCS] ......................................................................
Patch Set 15:
(1 comment)
https://review.coreboot.org/c/coreboot/+/44880/15/src/security/intel/txt/get... File src/security/intel/txt/getsec_enteraccs.S:
https://review.coreboot.org/c/coreboot/+/44880/15/src/security/intel/txt/get... PS15, Line 111: Disable all MTRRs
Ack
FYI that on DeltaLake, coreboot sets default memory attribute as WB instead of UC, through MSR 0x2ff.
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44880 )
Change subject: security/intel/txt: Improve MTRR setup for GETSEC[ENTERACCS] ......................................................................
Patch Set 15:
(1 comment)
https://review.coreboot.org/c/coreboot/+/44880/15/src/security/intel/txt/get... File src/security/intel/txt/getsec_enteraccs.S:
https://review.coreboot.org/c/coreboot/+/44880/15/src/security/intel/txt/get... PS15, Line 111: Disable all MTRRs
FYI that on DeltaLake, coreboot sets default memory attribute as WB instead of UC, through MSR 0x2ff.
Right. This code modifies the default memory attribute without restoring it. This is fixed in https://review.coreboot.org/c/coreboot/+/46375 (now merged)
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44880 )
Change subject: security/intel/txt: Improve MTRR setup for GETSEC[ENTERACCS] ......................................................................
Patch Set 16:
(1 comment)
https://review.coreboot.org/c/coreboot/+/44880/15/src/security/intel/txt/get... File src/security/intel/txt/getsec_enteraccs.S:
https://review.coreboot.org/c/coreboot/+/44880/15/src/security/intel/txt/get... PS15, Line 111: Disable all MTRRs
FYI that on DeltaLake, coreboot sets default memory attribute as WB instead of UC, through MSR 0x2ff […]
MSR 0x2ff is MTRR_DEF_TYPE_MSR, so yes, this can cause problems. This has been handled in CB:46375 though.
Philipp Deppenwiese has submitted this change. ( https://review.coreboot.org/c/coreboot/+/44880 )
Change subject: security/intel/txt: Improve MTRR setup for GETSEC[ENTERACCS] ......................................................................
security/intel/txt: Improve MTRR setup for GETSEC[ENTERACCS]
The BIOS ACM will check that enabled variable MTRRs do not cover more than the ACM's size, rounded up to 4 KiB. If that is not the case, launching the ACM will result in a lovely TXT reset. How boring.
The new algorithm simply performs a reverse bit scan in a loop, and allocates one MTRR for each set bit in the rounded-up size to cache. Before allocating anything, it checks if there are enough variable MTRRs; if not, it will refuse to cache anything. This will result in another TXT reset, initiated by the processor, with error type 5:
Load memory type error in Authenticated Code Execution Area.
This can only happen if the ACM has specific caching requirements that the current code does not know about, or something has been compromised. Therefore, causing a TXT reset should be a reasonable enough approach.
Also, disable all MTRRs before clearing the variable MTRRs and only enable them again once they have been set up with the new values.
Tested on Asrock B85M Pro4 with a BIOS ACM whose size is 101504 bytes. Without this patch, launching the ACM would result in a TXT reset. This no longer happens when this patch is applied.
Change-Id: I8d411f6450928357544be20250262c2005d1e75d Signed-off-by: Angel Pons th3fanbus@gmail.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/44880 Reviewed-by: Arthur Heymans arthur@aheymans.xyz Reviewed-by: Christian Walter christian.walter@9elements.com Tested-by: build bot (Jenkins) no-reply@coreboot.org --- M src/security/intel/txt/getsec_enteraccs.S 1 file changed, 87 insertions(+), 18 deletions(-)
Approvals: build bot (Jenkins): Verified Arthur Heymans: Looks good to me, approved Christian Walter: Looks good to me, approved
diff --git a/src/security/intel/txt/getsec_enteraccs.S b/src/security/intel/txt/getsec_enteraccs.S index 563dc08..be038b0 100644 --- a/src/security/intel/txt/getsec_enteraccs.S +++ b/src/security/intel/txt/getsec_enteraccs.S @@ -108,10 +108,10 @@ orl $(CR0_CD | CR0_NW), %eax movl %eax, %cr0
- /* Disable fixed MTRRs */ + /* Disable all MTRRs */ movl $(MTRR_DEF_TYPE_MSR), %ecx - rdmsr - andl $(~MTRR_DEF_TYPE_FIX_EN), %eax + xorl %eax, %eax + xorl %edx, %edx wrmsr
/* @@ -165,24 +165,93 @@ * Chapter A.1.1 * Intel TXT Software Development Guide (Document: 315168-015) */ - movl $(MTRR_PHYS_BASE(0)), %ecx - movl 12(%ebp), %eax /* %eax = acmbase */ - orl $(6), %eax /* MTRR_TYPE_WB */ - movl $0, %edx - wrmsr
- /* Round acmsize to next power of two. Required for MTRR programming. */ + /* + * Important note: The MTRRs must cache less than a page (4 KiB) + * of unused memory after the BIOS ACM. Failure to do so will + * result in a TXT reset with Class Code 5, Major Error Code 2. + * + * The caller must have checked that there are enough variable + * MTRRs to cache the ACM size prior to invoking this routine. + */ + + /* Determine size of AC module */ + movl 12(%ebp), %eax /* %eax = acmbase */ movl $1, %ebx - movl 16(%ebp), %ecx /* %ebx = acmsize */ - dec %ecx - bsr %ecx, %ecx /* find MSB */ - inc %ecx - shl %cl, %ebx - movl $(MTRR_PHYS_MASK(0)), %ecx - xorl %eax, %eax - subl %ebx, %eax /* %eax = 4GIB - log2_ceil(ACM SIZE) */ - orl $((1 << 11)), %eax /* MTRR_PHYS_MASK_VALID */ + movl 16(%ebp), %ebx /* %ebx = acmsize */ + + /* Round up to page size */ + addl $(0xfff), %ebx + andl $(~0xfff), %ebx /* Aligned to a page (4 KiB) */ + + /* + * Use XMM to store local variables. This code will need to be + * used in romstage, and CAR will have been torn down by then. + */ + movd %eax, %xmm0 /* XMM0: Base address of next MTRR */ + movd %ebx, %xmm1 /* XMM1: Remaining size to cache */ + + /* Get the number of variable MTRRs */ + movl $(MTRR_CAP_MSR), %ecx + rdmsr + andl $(0xff), %eax + + /* Initialize ECX */ + movl $(MTRR_PHYS_BASE(0)), %ecx + + jmp cond_allocate_var_mtrrs + +body_allocate_var_mtrrs: + + /* Program MTRR base */ + xorl %edx, %edx + movd %xmm0, %eax + orl $(MTRR_TYPE_WRBACK), %eax + wrmsr + incl %ecx /* Move index to MTRR_PHYS_MASK */ + + /* Temporarily transfer MSR index to EDX so that CL can be used */ + movl %ecx, %edx + + /* Determine next size to cache */ + bsr %ebx, %ecx + movl $(1), %ebx + shl %cl, %ebx /* Can only use CL here */ + + /* Restore ECX */ + movl %edx, %ecx + + /* Update saved base address */ + addl %ebx, %eax + movd %eax, %xmm0 + + /* Update saved remaining size */ + movd %xmm1, %eax + subl %ebx, %eax + movd %eax, %xmm1 + + /* Program MTRR mask */ movl MTRR_HIGH_MASK, %edx + xorl %eax, %eax + subl %ebx, %eax /* %eax = 4GIB - size to cache */ + orl $(MTRR_PHYS_MASK_VALID), %eax + wrmsr + incl %ecx /* Move index to next MTRR_PHYS_BASE */ + +cond_allocate_var_mtrrs: + + /* Check if we still need to cache something */ + movd %xmm1, %ebx + andl %ebx, %ebx + + jnz body_allocate_var_mtrrs + + /* + * Now that the variable MTRRs have been set up, enable them. + */ + movl $(MTRR_DEF_TYPE_MSR), %ecx + rdmsr + orl $(MTRR_DEF_TYPE_EN), %eax wrmsr
/* Enable cache - GPF# if not done */
-
Angel Pons (Code Review) -
Arthur Heymans (Code Review)
-
Christian Walter (Code Review)
-
Jonathan Zhang (Code Review)
-
Patrick Rudolph (Code Review)
-
Philipp Deppenwiese (Code Review)