Skip to contentSkip to conversation

x86/entry: Clarify the RF saving/restoring situation with SYSCALL/SYSRET

PATCH In review Borislav Petkov 3 participants 6 messages Aug 3, 2016 – Aug 10, 2016

Subsystems (2)
lore
Share & tools
lore

Borislav Petkov

author maintainer
#0 lore raw

PATCH 1/1 x86/entry: Clarify the RF saving/restoring situation with SYSCALL/SYSRET

From: Borislav Petkov <[email protected]>

Clarify why exactly RF cannot be restored properly by SYSRET to avoid confusion.

No functionality change.

---
 arch/x86/entry/entry_64.S | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)
arch/x86/entry/entry_64.S +9 -5
index 8956eae04c25..80ad6d0fe38b 100644
@@ -288,11 +288,15 @@ return_from_SYSCALL_64:
 	jne	opportunistic_sysret_failed
 
 	/*
-	 * SYSRET can't restore RF.  SYSRET can restore TF, but unlike IRET,
-	 * restoring TF results in a trap from userspace immediately after
-	 * SYSRET.  This would cause an infinite loop whenever #DB happens
-	 * with register state that satisfies the opportunistic SYSRET
-	 * conditions.  For example, single-stepping this user code:
+	 * SYSCALL clears RF when it saves rFLAGS in R11 so SYSRET cannot
+	 * restore RF properly. If the slowpath sets it for whatever reason, we
+	 * need to restore it correctly.
+	 *
+	 * SYSRET can restore TF, but unlike IRET, restoring TF results in a
+	 * trap from userspace immediately after SYSRET.  This would cause an
+	 * infinite loop whenever #DB happens with register state that satisfies
+	 * the opportunistic SYSRET conditions.  For example, single-stepping
+	 * this user code:
 	 *
 	 *           movq	$stuck_here, %rcx
 	 *           pushfq
Signature · 1 line
2.8.4
Acked-by on v1 1/1
#1 lore raw
Quoted 18 lines · Borislav Petkov

From: Borislav Petkov <[email protected]>

Clarify why exactly RF cannot be restored properly by SYSRET to avoid confusion.

No functionality change.

---
 arch/x86/entry/entry_64.S | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)
arch/x86/entry/entry_64.S +0 -0
index 8956eae04c25..80ad6d0fe38b 100644

arch/x86/entry/entry_64.S return_from_SYSCALL_64: · line 291

Quoted 9 lines
arch/x86/entry/entry_64.S +1 -5
@@ -288,11 +288,15 @@ return_from_SYSCALL_64:
        jne     opportunistic_sysret_failed
 
        /*
-        * SYSRET can't restore RF.  SYSRET can restore TF, but unlike IRET,
-        * restoring TF results in a trap from userspace immediately after
-        * SYSRET.  This would cause an infinite loop whenever #DB happens
-        * with register state that satisfies the opportunistic SYSRET
-        * conditions.  For example, single-stepping this user code:
+        * SYSCALL clears RF when it saves rFLAGS in R11 so SYSRET cannot

I would change "so" and "and" -- the CPU designers could have make SYSRET restore RF, but they chose not to.

Other than that substitution:

author maintainer Acked-by Andy Lutomirski
#2 lore raw
Quoted 2 lines · Andy Lutomirski

I would change "so" and "and" -- the CPU designers could have make SYSRET restore RF, but they chose not to.

I'm assuming the reasoning behind it was that you should be able to break after SYSRET but then again, the kernel could've been left in control of that bit and set it or clear it however it likes before SYSRETing.

Oh well.

Quoted 3 lines

Other than that substitution:

Thanks.

Here's v1.1:

---
From: Borislav Petkov <[email protected]>
Date: Wed, 3 Aug 2016 12:17:10 +0200
Subject: [PATCH -v1.1] x86/entry: Clarify the RF saving/restoring situation with SYSCALL/SYSRET

Clarify why exactly RF cannot be restored properly by SYSRET to avoid confusion.

No functionality change.

---
 arch/x86/entry/entry_64.S | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)
arch/x86/entry/entry_64.S +9 -5
index 8956eae04c25..b4b546bb23c3 100644
@@ -288,11 +288,15 @@ return_from_SYSCALL_64:
 	jne	opportunistic_sysret_failed
 
 	/*
-	 * SYSRET can't restore RF.  SYSRET can restore TF, but unlike IRET,
-	 * restoring TF results in a trap from userspace immediately after
-	 * SYSRET.  This would cause an infinite loop whenever #DB happens
-	 * with register state that satisfies the opportunistic SYSRET
-	 * conditions.  For example, single-stepping this user code:
+	 * SYSCALL clears RF when it saves rFLAGS in R11 and SYSRET cannot
+	 * restore RF properly. If the slowpath sets it for whatever reason, we
+	 * need to restore it correctly.
+	 *
+	 * SYSRET can restore TF, but unlike IRET, restoring TF results in a
+	 * trap from userspace immediately after SYSRET.  This would cause an
+	 * infinite loop whenever #DB happens with register state that satisfies
+	 * the opportunistic SYSRET conditions.  For example, single-stepping
+	 * this user code:
 	 *
 	 *           movq	$stuck_here, %rcx
 	 *           pushfq
Signature · 9 lines
2.8.4


-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.
--
#3 lore raw
Quoted 10 lines · Borislav Petkov
Quoted 2 lines · Andy Lutomirski

I would change "so" and "and" -- the CPU designers could have make SYSRET restore RF, but they chose not to.

I'm assuming the reasoning behind it was that you should be able to break after SYSRET but then again, the kernel could've been left in control of that bit and set it or clear it however it likes before SYSRETing.

Oh well.

AFAICT the AMD people didn't think of any use cases involving doing anything interesting between SYSCALL and SYSRET. Witness the sysret_ss_attrs goof, for example: apparently SYSCALL; context switch; IRET; interrupt; context switch; SYSRET didn't occur to AMD as a valid use case.

--Andy

author maintainer
#4 lore raw
Quoted 5 lines · Andy Lutomirski

AFAICT the AMD people didn't think of any use cases involving doing anything interesting between SYSCALL and SYSRET. Witness the sysret_ss_attrs goof, for example: apparently SYSCALL; context switch; IRET; interrupt; context switch; SYSRET didn't occur to AMD as a valid use case.

/me hands Andy a time machine to go fix this properly, before so much silicon ships.

Signature · 5 lines
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.
--
#5 lore raw
In reply to Borislav Petkov
x86/entry: Clarify the RF saving/restoring situation with SYSCALL/SYSRET
Commit-ID:  3e035305875cfa8a58c1ca573d0cfa6a7f201f27
Gitweb:     http://git.kernel.org/tip/3e035305875cfa8a58c1ca573d0cfa6a7f201f27
Author:     Borislav Petkov <[email protected]>
AuthorDate: Wed, 3 Aug 2016 19:14:29 +0200
Committer:  Ingo Molnar <[email protected]>
CommitDate: Wed, 10 Aug 2016 15:53:43 +0200

x86/entry: Clarify the RF saving/restoring situation with SYSCALL/SYSRET

Clarify why exactly RF cannot be restored properly by SYSRET to avoid confusion.

No functionality change.

Cc Andy Lutomirski <[email protected]>
Cc Borislav Petkov <[email protected]>
Cc Brian Gerst <[email protected]>
Cc Denys Vlasenko <[email protected]>
Cc H. Peter Anvin <[email protected]>
Cc Josh Poimboeuf <[email protected]>
Cc Linus Torvalds <[email protected]>
Cc Peter Zijlstra <[email protected]>
Cc Thomas Gleixner <[email protected]>
---
 arch/x86/entry/entry_64.S | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)
arch/x86/entry/entry_64.S +9 -5
index 9f85827..d172c61 100644
@@ -288,11 +288,15 @@ return_from_SYSCALL_64:
 	jne	opportunistic_sysret_failed
 
 	/*
-	 * SYSRET can't restore RF.  SYSRET can restore TF, but unlike IRET,
-	 * restoring TF results in a trap from userspace immediately after
-	 * SYSRET.  This would cause an infinite loop whenever #DB happens
-	 * with register state that satisfies the opportunistic SYSRET
-	 * conditions.  For example, single-stepping this user code:
+	 * SYSCALL clears RF when it saves RFLAGS in R11 and SYSRET cannot
+	 * restore RF properly. If the slowpath sets it for whatever reason, we
+	 * need to restore it correctly.
+	 *
+	 * SYSRET can restore TF, but unlike IRET, restoring TF results in a
+	 * trap from userspace immediately after SYSRET.  This would cause an
+	 * infinite loop whenever #DB happens with register state that satisfies
+	 * the opportunistic SYSRET conditions.  For example, single-stepping
+	 * this user code:
 	 *
 	 *           movq	$stuck_here, %rcx
 	 *           pushfq