All of lore.kernel.org
 help / color / mirror / Atom feed
From: Andrew Cooper <andrew.cooper3@citrix.com>
To: Xen-devel <xen-devel@lists.xenproject.org>
Cc: "Andrew Cooper" <andrew.cooper3@citrix.com>,
	"Jan Beulich" <JBeulich@suse.com>,
	"Roger Pau Monné" <roger.pau@citrix.com>, "Wei Liu" <wl@xen.org>,
	"Marek Marczykowski-Górecki" <marmarek@invisiblethingslab.com>
Subject: [PATCH] x86/cet: Fix shskt manipulation error with BUGFRAME_{warn,run_fn}
Date: Thu, 12 Aug 2021 18:03:50 +0100	[thread overview]
Message-ID: <20210812170350.23543-1-andrew.cooper3@citrix.com> (raw)

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



             reply	other threads:[~2021-08-12 17:10 UTC|newest]

Thread overview: 6+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-08-12 17:03 Andrew Cooper [this message]
2021-08-12 18:16 ` [PATCH] x86/cet: Fix shskt manipulation error with BUGFRAME_{warn,run_fn} Marek Marczykowski-Górecki
2021-08-13 14:00 ` Jan Beulich
2021-08-16 10:31   ` Andrew Cooper
2021-08-17  9:10 ` Jan Beulich
2021-08-17 10:31   ` Andrew Cooper

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20210812170350.23543-1-andrew.cooper3@citrix.com \
    --to=andrew.cooper3@citrix.com \
    --cc=JBeulich@suse.com \
    --cc=marmarek@invisiblethingslab.com \
    --cc=roger.pau@citrix.com \
    --cc=wl@xen.org \
    --cc=xen-devel@lists.xenproject.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.