Messages in this thread Patch in this message | | | Date | Thu, 13 Jan 2022 10:54:39 -0800 | Subject | Re: [PATCH] x86/entry_32: Fix segment exceptions | From | Andy Lutomirski <> |
| |
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@infradead.org> > 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@google.com>. > > [ bp: Add EX_REG_GS too. ] > > Fixes: aa93e2ad7464 ("x86/entry_32: Remove .fixup usage") > Reported-by: kernel test robot <oliver.sang@intel.com> > Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org> > Signed-off-by: Borislav Petkov <bp@suse.de> > 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?
| |