[PATCH] x86/cet: Fix shskt manipulation error with BUGFRAME_{warn,run_fn}

[PATCH] x86/cet: Fix shskt manipulation error with BUGFRAME_{warn,run_fn}

[Andrew Cooper]

This was a clear oversight in the original CET work.  The BUGFRAME_run_fn and
BUGFRAME_warn paths update regs->rip without an equivlenet adjustment to the
shadow stack, causes IRET to suffer #CP due to the mismatch.

One subtle, and therefore fragile, aspect of extable_shstk_fixup() was that it
required regs->rip to have its old value as a cross-check that the correct
word in the shadow stack was being adjusted.

Rework extable_shstk_fixup() into fixup_exception_return() which takes
ownership of the update to both the regular and shadow stacks, ensuring that
the regs->rip update is ordered suitably.

Use the new fixup_exception_return() for BUGFRAME_run_fn and BUGFRAME_warn to
ensure that the shadow stack is updated too.

Fixes: 209fb9919b50 ("x86/extable: Adjust extable handling to be shadow stack compatible")
Reported-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Wei Liu <wl@xen.org>
CC: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>

Backport to 4.14

Only compile tested so far.  My one CET-SS machine is in use for other
purposes right now.

I'm not a massive fan of the large ifdef area.  The logic could be rearranged
to use IS_ENABLED(CONFIG_XEN_SHSTK) by indenting most of the function, but I
can't see any way to drop the goto's, and this is certainly the least-invasive
diff.
---
 xen/arch/x86/traps.c | 23 ++++++++++++++---------
 1 file changed, 14 insertions(+), 9 deletions(-)

diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c
index e60af16ddd8c..30eefbad4863 100644
--- a/xen/arch/x86/traps.c
+++ b/xen/arch/x86/traps.c
@@ -777,13 +777,15 @@ static void do_reserved_trap(struct cpu_user_regs *regs)
           trapnr, vec_name(trapnr), regs->error_code);
 }
 
-static void extable_shstk_fixup(struct cpu_user_regs *regs, unsigned long fixup)
+static void fixup_exception_return(struct cpu_user_regs *regs,
+                                   unsigned long fixup)
 {
+#ifdef CONFIG_XEN_SHSTK
     unsigned long ssp, *ptr, *base;
 
     asm ( "rdsspq %0" : "=r" (ssp) : "0" (1) );
     if ( ssp == 1 )
-        return;
+        goto shstk_done;
 
     ptr = _p(ssp);
     base = _p(get_shstk_bottom(ssp));
@@ -814,7 +816,7 @@ static void extable_shstk_fixup(struct cpu_user_regs *regs, unsigned long fixup)
             asm ( "wrssq %[fix], %[stk]"
                   : [stk] "=m" (ptr[0])
                   : [fix] "r" (fixup) );
-            return;
+            goto shstk_done;
         }
     }
 
@@ -824,6 +826,12 @@ static void extable_shstk_fixup(struct cpu_user_regs *regs, unsigned long fixup)
      * executing the interrupted context.
      */
     BUG();
+
+ shstk_done:
+#endif /* CONFIG_XEN_SHSTK */
+
+    /* Fixup the regular stack. */
+    regs->rip = fixup;
 }
 
 static bool extable_fixup(struct cpu_user_regs *regs, bool print)
@@ -842,10 +850,7 @@ static bool extable_fixup(struct cpu_user_regs *regs, bool print)
                vec_name(regs->entry_vector), regs->error_code,
                _p(regs->rip), _p(regs->rip), _p(fixup));
 
-    if ( IS_ENABLED(CONFIG_XEN_SHSTK) )
-        extable_shstk_fixup(regs, fixup);
-
-    regs->rip = fixup;
+    fixup_exception_return(regs, fixup);
     this_cpu(last_extable_addr) = regs->rip;
 
     return true;
@@ -1138,7 +1143,7 @@ void do_invalid_op(struct cpu_user_regs *regs)
         void (*fn)(struct cpu_user_regs *) = bug_ptr(bug);
 
         fn(regs);
-        regs->rip = (unsigned long)eip;
+        fixup_exception_return(regs, (unsigned long)eip);
         return;
     }
 
@@ -1159,7 +1164,7 @@ void do_invalid_op(struct cpu_user_regs *regs)
     case BUGFRAME_warn:
         printk("Xen WARN at %s%s:%d\n", prefix, filename, lineno);
         show_execution_state(regs);
-        regs->rip = (unsigned long)eip;
+        fixup_exception_return(regs, (unsigned long)eip);
         return;
 
     case BUGFRAME_bug:
-- 
2.11.0

Re: [PATCH] x86/cet: Fix shskt manipulation error with BUGFRAME_{warn,run_fn}

[Marek Marczykowski-Górecki]

[-- Attachment #1: Type: text/plain, Size: 1284 bytes --]

On Thu, Aug 12, 2021 at 06:03:50PM +0100, Andrew Cooper wrote:
> This was a clear oversight in the original CET work.  The BUGFRAME_run_fn and
> BUGFRAME_warn paths update regs->rip without an equivlenet adjustment to the
> shadow stack, causes IRET to suffer #CP due to the mismatch.
> 
> One subtle, and therefore fragile, aspect of extable_shstk_fixup() was that it
> required regs->rip to have its old value as a cross-check that the correct
> word in the shadow stack was being adjusted.
> 
> Rework extable_shstk_fixup() into fixup_exception_return() which takes
> ownership of the update to both the regular and shadow stacks, ensuring that
> the regs->rip update is ordered suitably.
> 
> Use the new fixup_exception_return() for BUGFRAME_run_fn and BUGFRAME_warn to
> ensure that the shadow stack is updated too.
> 
> Fixes: 209fb9919b50 ("x86/extable: Adjust extable handling to be shadow stack compatible")
> Reported-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

With this path, I don't observe the crash anymore. Thanks!

Tested-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>

-- 
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] x86/cet: Fix shskt manipulation error with BUGFRAME_{warn,run_fn}

[Jan Beulich]

On 12.08.2021 19:03, Andrew Cooper wrote:
> This was a clear oversight in the original CET work.  The BUGFRAME_run_fn and
> BUGFRAME_warn paths update regs->rip without an equivlenet adjustment to the
> shadow stack, causes IRET to suffer #CP due to the mismatch.
> 
> One subtle, and therefore fragile, aspect of extable_shstk_fixup() was that it
> required regs->rip to have its old value as a cross-check that the correct
> word in the shadow stack was being adjusted.
> 
> Rework extable_shstk_fixup() into fixup_exception_return() which takes
> ownership of the update to both the regular and shadow stacks, ensuring that
> the regs->rip update is ordered suitably.
> 
> Use the new fixup_exception_return() for BUGFRAME_run_fn and BUGFRAME_warn to
> ensure that the shadow stack is updated too.
> 
> Fixes: 209fb9919b50 ("x86/extable: Adjust extable handling to be shadow stack compatible")
> Reported-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Jan Beulich <jbeulich@suse.com>

> I'm not a massive fan of the large ifdef area.  The logic could be rearranged
> to use IS_ENABLED(CONFIG_XEN_SHSTK) by indenting most of the function, but I
> can't see any way to drop the goto's, and this is certainly the least-invasive
> diff.

It's not really neat, but we've got worse code elsewhere.

I wonder whether gdb_arch_resume() and gdb_arch_write_reg() also
need some sort of similar adjustment.

Jan

Re: [PATCH] x86/cet: Fix shskt manipulation error with BUGFRAME_{warn,run_fn}

[Andrew Cooper]

On 13/08/2021 15:00, Jan Beulich wrote:
> On 12.08.2021 19:03, Andrew Cooper wrote:
>> This was a clear oversight in the original CET work.  The BUGFRAME_run_fn and
>> BUGFRAME_warn paths update regs->rip without an equivlenet adjustment to the
>> shadow stack, causes IRET to suffer #CP due to the mismatch.
>>
>> One subtle, and therefore fragile, aspect of extable_shstk_fixup() was that it
>> required regs->rip to have its old value as a cross-check that the correct
>> word in the shadow stack was being adjusted.
>>
>> Rework extable_shstk_fixup() into fixup_exception_return() which takes
>> ownership of the update to both the regular and shadow stacks, ensuring that
>> the regs->rip update is ordered suitably.
>>
>> Use the new fixup_exception_return() for BUGFRAME_run_fn and BUGFRAME_warn to
>> ensure that the shadow stack is updated too.
>>
>> Fixes: 209fb9919b50 ("x86/extable: Adjust extable handling to be shadow stack compatible")
>> Reported-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Reviewed-by: Jan Beulich <jbeulich@suse.com>

Thanks.

>
>> I'm not a massive fan of the large ifdef area.  The logic could be rearranged
>> to use IS_ENABLED(CONFIG_XEN_SHSTK) by indenting most of the function, but I
>> can't see any way to drop the goto's, and this is certainly the least-invasive
>> diff.
> It's not really neat, but we've got worse code elsewhere.
>
> I wonder whether gdb_arch_resume() and gdb_arch_write_reg() also
> need some sort of similar adjustment.

Hmm.

So there's nothing we can do right now, because GDB has yet to gain an
understanding of shadow stacks (or rather, the prototype so far is still
under discussion on LKML, given that CET support has yet to be included
into Linux).

Beyond that, I haven't seen gdbstub modified in a decade, nor have I
found anyone who's used it.  I'd be astounded if it actually works.

I think its fine for people using GDB to know that they need to turn off
CET first, but I don't expect anyone to have a pleasant time trying to
use gdbstub to begin with in Xen.

~Andrew

Re: [PATCH] x86/cet: Fix shskt manipulation error with BUGFRAME_{warn,run_fn}

[Jan Beulich]

On 12.08.2021 19:03, Andrew Cooper wrote:
> I'm not a massive fan of the large ifdef area.  The logic could be rearranged
> to use IS_ENABLED(CONFIG_XEN_SHSTK) by indenting most of the function, but I
> can't see any way to drop the goto's, and this is certainly the least-invasive
> diff.

So perhaps the build failure I've just run into (also apparently spotted
by osstest) suggests to actually do so? The alternative would seem to be
to widen the #ifdef in get_shstk_bottom() to cover the function as a
whole ...

Jan

Re: [PATCH] x86/cet: Fix shskt manipulation error with BUGFRAME_{warn,run_fn}

[Andrew Cooper]

On 17/08/2021 10:10, Jan Beulich wrote:
> On 12.08.2021 19:03, Andrew Cooper wrote:
>> I'm not a massive fan of the large ifdef area.  The logic could be rearranged
>> to use IS_ENABLED(CONFIG_XEN_SHSTK) by indenting most of the function, but I
>> can't see any way to drop the goto's, and this is certainly the least-invasive
>> diff.
> So perhaps the build failure I've just run into (also apparently spotted
> by osstest) suggests to actually do so? The alternative would seem to be
> to widen the #ifdef in get_shstk_bottom() to cover the function as a
> whole ...

Let me see how the options look...

~Andrew