Messages in this thread Patch in this message | | | Subject | Re: [PATCH 0/4] x86/module: Out-of-tree module decode and sanitize | From | Andrew Cooper <> | Date | Tue, 7 Apr 2020 22:21:02 +0100 |
| |
On 07/04/2020 21:45, Peter Zijlstra wrote: >> POPF: Don't really want someone being able to set IOPL3. However, this >> might quite easily show up as a false positive depending on how the >> irqsafe infrastructure gets inlined. > local_irq_restore() will be a POPF :/
Ok. Something to consider in an orthogonal direction. A while ago, I put this into Xen as a security fix:
iret_exit_to_guest: andl $~(X86_EFLAGS_IOPL|X86_EFLAGS_NT|X86_EFLAGS_VM),24(%rsp) orl $X86_EFLAGS_IF,24(%rsp) addq $8,%rsp .Lft0: iretq
which unconditionally fixes up the unsafe flags even if something manages to slips through (e.g. local_irq_restore() against stack rubble). It turns out that it has saved us several CVEs in the intervening time.
Is this the kind of things the hardening folk would be interested in?
> --- a/arch/x86/kernel/module.c > +++ b/arch/x86/kernel/module.c > @@ -282,6 +282,68 @@ static bool insn_is_mov_DRn(struct insn > return false; > } > > +static bool insn_is_GDT_modifier(struct insn *insn) > +{ > + u8 modrm = insn->modrm.bytes[0]; > + u8 modrm_mod = X86_MODRM_MOD(modrm); > + u8 modrm_reg = X86_MODRM_REG(modrm); > + > + if (insn->opcode.bytes[0] != 0x0f) > + return false; > + > + switch (insn->opcode.bytes[1]) { > + case 0x00: /* Grp6 */ > + switch (modrm_reg) { > + /* case 0x0: SLDT */ > + case 0x2: /* LLDT */ > + case 0x3: /* LTR */ > + return true;
Come to think of it, if you include the Sxxx variants, a sufficiently clever compiler can collapse this entire switch statement into a single "and $~3, modrm_reg" instruction, rather than being forced to use "and $~1, modrm_reg; cmp $2, ...".
Probably on the extreme end of micro-optimising however.
~Andrew
| |