Re: [PATCH] x86/tboot: Drop mfn_in_guarded_stack()

[Jan Beulich]

On 26.07.2022 14:25, Andrew Cooper wrote:
> To support CET Shadow Stacks, guard pages changed from being holes to being
> read-only.  As such, they can be read.
> 
> Moreover, they should be included in the integrity check.

As long as they're non-present mappings, I don't think they should be
included here, so - not being a native speaker - I'm not sure about
"moreover".

> Fixes: 60016604739b ("x86/shstk: Rework the stack layout to support shadow 
> stacks")
> Signed-off-by: Andrew Cooper 

Reviewed-by: Jan Beulich 

You should have included the three TXT reviewers: I would have been
curious who, if anyone, would actually have responded.

Jan

[PATCH] x86/tboot: Drop mfn_in_guarded_stack()

[Andrew Cooper]

To support CET Shadow Stacks, guard pages changed from being holes to being
read-only.  As such, they can be read.

Moreover, they should be included in the integrity check.

Fixes: 60016604739b ("x86/shstk: Rework the stack layout to support shadow 
stacks")
Signed-off-by: Andrew Cooper 
---
CC: Jan Beulich 
CC: Roger Pau Monné 
CC: Wei Liu 
---
 xen/arch/x86/tboot.c | 33 +
 1 file changed, 1 insertion(+), 32 deletions(-)

diff --git a/xen/arch/x86/tboot.c b/xen/arch/x86/tboot.c
index fe1abfdf08ff..03098450f788 100644
--- a/xen/arch/x86/tboot.c
+++ b/xen/arch/x86/tboot.c
@@ -242,29 +242,6 @@ static void tboot_gen_domain_integrity(const uint8_t 
key[TB_KEY_SIZE],
 memset(, 0, sizeof(ctx));
 }
 
-/*
- * For stack overflow detection in debug build, a guard page is set up.
- * This fn is used to detect whether a page is in the guarded pages for
- * the above reason.
- */
-static int mfn_in_guarded_stack(unsigned long mfn)
-{
-void *p;
-int i;
-
-for ( i = 0; i < nr_cpu_ids; i++ )
-{
-if ( !stack_base[i] )
-continue;
-p = (void *)((unsigned long)stack_base[i] + STACK_SIZE -
- PRIMARY_STACK_SIZE - PAGE_SIZE);
-if ( mfn == virt_to_mfn(p) )
-return -1;
-}
-
-return 0;
-}
-
 static void tboot_gen_xenheap_integrity(const uint8_t key[TB_KEY_SIZE],
 vmac_t *mac)
 {
@@ -288,15 +265,7 @@ static void tboot_gen_xenheap_integrity(const uint8_t 
key[TB_KEY_SIZE],
 continue; /* skip tboot and its page tables */
 
 if ( is_page_in_use(page) && is_special_page(page) )
-{
-void *pg;
-
-if ( mfn_in_guarded_stack(mfn) )
-continue; /* skip guard stack, see memguard_guard_stack() in 
mm.c */
-
-pg = mfn_to_virt(mfn);
-vmac_update((uint8_t *)pg, PAGE_SIZE, );
-}
+vmac_update(mfn_to_virt(mfn), PAGE_SIZE, );
 }
 *mac = vmac(NULL, 0, nonce, NULL, );
 
-- 
2.11.0