Re: [PATCH] x86/entry_32: Fix segment exceptions

From: Andy Lutomirski
Date: Thu Jan 13 2022 - 13:54:53 EST

On 1/12/22 07:42, Borislav Petkov wrote:
On Wed, Jan 12, 2022 at 11:55:41AM +0100, Peter Zijlstra wrote:
Full and proper patch below. Boris, if you could merge in x86/core that
branch should then be ready for a pull req.

I've got this as the final version. Scream if something's wrong.

AAAAAAAAAAAAAAAAAAAAAHHHHHHHHHHHHHHHHHHHHHH!!!!!!!!!!!



---
From: Peter Zijlstra <peterz@xxxxxxxxxxxxx>
Date: Tue, 11 Jan 2022 12:11:14 +0100
Subject: [PATCH] x86/entry_32: Fix segment exceptions

The LKP robot reported that commit in Fixes: caused a failure. Turns out
the ldt_gdt_32 selftest turns into an infinite loop trying to clear the
segment.

As discovered by Sean, what happens is that PARANOID_EXIT_TO_KERNEL_MODE
in the handle_exception_return path overwrites the entry stack data with
the task stack data, restoring the "bad" segment value.

Instead of having the exception retry the instruction, have it emulate
the full instruction. Replace EX_TYPE_POP_ZERO with EX_TYPE_POP_REG
which will do the equivalent of: POP %reg; MOV $imm, %reg.

In order to encode the segment registers, add them as registers 8-11 for
32-bit.

By setting regs->[defg]s the (nested) RESTORE_REGS will pop this value
at the end of the exception handler and by increasing regs->sp, it will
have skipped the stack slot.

This was debugged by Sean Christopherson <seanjc@xxxxxxxxxx>.

[ bp: Add EX_REG_GS too. ]

Fixes: aa93e2ad7464 ("x86/entry_32: Remove .fixup usage")
Reported-by: kernel test robot <oliver.sang@xxxxxxxxx>
Signed-off-by: Peter Zijlstra (Intel) <peterz@xxxxxxxxxxxxx>
Signed-off-by: Borislav Petkov <bp@xxxxxxx>
Link: https://lore.kernel.org/r/Yd1l0gInc4zRcnt/@hirez.programming.kicks-ass.net
---
arch/x86/entry/entry_32.S | 13 +++++++++----
arch/x86/include/asm/extable_fixup_types.h | 11 ++++++++++-
arch/x86/lib/insn-eval.c | 5 +++++
arch/x86/mm/extable.c | 17 +++--------------
4 files changed, 27 insertions(+), 19 deletions(-)

diff --git a/arch/x86/entry/entry_32.S b/arch/x86/entry/entry_32.S
index e0a95d8a6553..a7ec22b1d06c 100644
--- a/arch/x86/entry/entry_32.S
+++ b/arch/x86/entry/entry_32.S
@@ -268,11 +268,16 @@
1: popl %ds
2: popl %es
3: popl %fs
- addl $(4 + \pop), %esp /* pop the unused "gs" slot */
+4: addl $(4 + \pop), %esp /* pop the unused "gs" slot */
IRET_FRAME
- _ASM_EXTABLE_TYPE(1b, 1b, EX_TYPE_POP_ZERO)
- _ASM_EXTABLE_TYPE(2b, 2b, EX_TYPE_POP_ZERO)
- _ASM_EXTABLE_TYPE(3b, 3b, EX_TYPE_POP_ZERO)
+
+ /*
+ * There is no _ASM_EXTABLE_TYPE_REG() for ASM, however since this is
+ * ASM the registers are known and we can trivially hard-code them.
+ */
+ _ASM_EXTABLE_TYPE(1b, 2b, EX_TYPE_POP_ZERO|EX_REG_DS)
+ _ASM_EXTABLE_TYPE(2b, 3b, EX_TYPE_POP_ZERO|EX_REG_ES)
+ _ASM_EXTABLE_TYPE(3b, 4b, EX_TYPE_POP_ZERO|EX_REG_FS)

Aside from POP_ZERO being a bit mystifying to a naive reader...

.endm
.macro RESTORE_ALL_NMI cr3_reg:req pop=0
diff --git a/arch/x86/include/asm/extable_fixup_types.h b/arch/x86/include/asm/extable_fixup_types.h
index b5ab333e064a..503622627400 100644
--- a/arch/x86/include/asm/extable_fixup_types.h
+++ b/arch/x86/include/asm/extable_fixup_types.h
@@ -16,9 +16,16 @@
#define EX_DATA_FLAG_SHIFT 12
#define EX_DATA_IMM_SHIFT 16
+#define EX_DATA_REG(reg) ((reg) << EX_DATA_REG_SHIFT)
#define EX_DATA_FLAG(flag) ((flag) << EX_DATA_FLAG_SHIFT)
#define EX_DATA_IMM(imm) ((imm) << EX_DATA_IMM_SHIFT)
+/* segment regs */
+#define EX_REG_DS EX_DATA_REG(8)
+#define EX_REG_ES EX_DATA_REG(9)
+#define EX_REG_FS EX_DATA_REG(10)

These three seem likely to work

+#define EX_REG_GS EX_DATA_REG(11)

But not this one.

+
/* flags */
#define EX_FLAG_CLEAR_AX EX_DATA_FLAG(1)
#define EX_FLAG_CLEAR_DX EX_DATA_FLAG(2)
@@ -41,7 +48,9 @@
#define EX_TYPE_RDMSR_IN_MCE 13
#define EX_TYPE_DEFAULT_MCE_SAFE 14
#define EX_TYPE_FAULT_MCE_SAFE 15
-#define EX_TYPE_POP_ZERO 16
+
+#define EX_TYPE_POP_REG 16 /* sp += sizeof(long) */
+#define EX_TYPE_POP_ZERO (EX_TYPE_POP_REG | EX_DATA_IMM(0))
#define EX_TYPE_IMM_REG 17 /* reg := (long)imm */
#define EX_TYPE_EFAULT_REG (EX_TYPE_IMM_REG | EX_DATA_IMM(-EFAULT))
diff --git a/arch/x86/lib/insn-eval.c b/arch/x86/lib/insn-eval.c
index 7760d228041b..c8a962c2e653 100644
--- a/arch/x86/lib/insn-eval.c
+++ b/arch/x86/lib/insn-eval.c
@@ -430,6 +430,11 @@ static const int pt_regoff[] = {
offsetof(struct pt_regs, r13),
offsetof(struct pt_regs, r14),
offsetof(struct pt_regs, r15),
+#else
+ offsetof(struct pt_regs, ds),
+ offsetof(struct pt_regs, es),
+ offsetof(struct pt_regs, fs),
+ offsetof(struct pt_regs, gs),

See the comment in asm/ptrace.h over gs :)

Fortunately nothing uses EX_REG_GS. Maybe just remove all the gs bits and leave the rest alone?