On Thu, 2017-06-08 at 20:38 +0200, Borislav Petkov wrote:
> On Fri, May 05, 2017 at 11:17:19AM -0700, Ricardo Neri wrote:
> > The feature User-Mode Instruction Prevention present in recent Intel
> > processor prevents a group of instructions from being executed with
> > CPL > 0. Otherwise, a general protection fault is issued.
>
> This is one of the best opening paragraphs of a commit message I've
> read this year! This is how you open: short, succinct, to the point, no
> marketing bullshit. Good!
Thanks you!
>
> > Rather than relaying this fault to the user space (in the form of a SIGSEGV
> > signal), the instructions protected by UMIP can be emulated to provide
> > dummy results. This allows to conserve the current kernel behavior and not
> > reveal the system resources that UMIP intends to protect (the global
> > descriptor and interrupt descriptor tables, the segment selectors of the
> > local descriptor table and the task state and the machine status word).
> >
> > This emulation is needed because certain applications (e.g., WineHQ and
> > DOSEMU2) rely on this subset of instructions to function.
> >
> > The instructions protected by UMIP can be split in two groups. Those who
>
> s/who/which/
I will correct.
>
> > return a kernel memory address (sgdt and sidt) and those who return a
>
> ditto.
I will correct here also.
>
> > value (sldt, str and smsw).
> >
> > For the instructions that return a kernel memory address, applications
> > such as WineHQ rely on the result being located in the kernel memory space.
> > The result is emulated as a hard-coded value that, lies close to the top
> > of the kernel memory. The limit for the GDT and the IDT are set to zero.
>
> Nice.
>
> > Given that sldt and str are not used in common in programs supported by
>
> You wanna say "in common programs" here? Or "not commonly used in programs" ?
I will rephrase this comment.
>
> > WineHQ and DOSEMU2, they are not emulated.
> >
> > The instruction smsw is emulated to return the value that the register CR0
> > has at boot time as set in the head_32.
> >
> > Care is taken to appropriately emulate the results when segmentation is
> > used. This is, rather than relying on USER_DS and USER_CS, the function
>
> "That is,... "
I will correct it.
>
> > insn_get_addr_ref() inspects the segment descriptor pointed by the
> > registers in pt_regs. This ensures that we correctly obtain the segment
> > base address and the address and operand sizes even if the user space
> > application uses local descriptor table.
>
> Btw, I could very well use all that nice explanation in umip.c too so
> that the high-level behavior is documented.
Sure, I will include a high-level description in the file itself.
>
> > Cc: Andy Lutomirski
> > Cc: Andrew Morton
> > Cc: H. Peter Anvin
> > Cc: Borislav Petkov
> > Cc: Brian Gerst
> > Cc: Chen Yucong
> > Cc: Chris Metcalf
> > Cc: Dave Hansen
> > Cc: Fenghua Yu
> > Cc: Huang Rui
> > Cc: Jiri Slaby
> > Cc: Jonathan Corbet
> > Cc: Michael S. Tsirkin
> > Cc: Paul Gortmaker
> > Cc: Peter Zijlstra
> > Cc: Ravi V. Shankar
> > Cc: Shuah Khan
> > Cc: Vlastimil Babka
> > Cc: Tony Luck
> > Cc: Paolo Bonzini
> > Cc: Liang Z. Li
> > Cc: Alexandre Julliard
> > Cc: Stas Sergeev
> > Cc: x...@kernel.org
> > Cc: linux-msdos@vger.kernel.org
> > Signed-off-by: Ricardo Neri
> > ---
> > arch/x86/include/asm/umip.h | 15 +++
> > arch/x86/kernel/Makefile| 1 +
> > arch/x86/kernel/umip.c | 245
> >
> > 3 files changed, 261 insertions(+)
> > create mode 100644 arch/x86/include/asm/umip.h
> > create mode 100644 arch/x86/kernel/umip.c
> >
> > diff --git a/arch/x86/include/asm/umip.h b/arch/x86/include/asm/umip.h
> > new file mode 100644
> > index 000..077b236
> > --- /dev/null
> > +++ b/arch/x86/include/asm/umip.h
> > @@ -0,0 +1,15 @@
> > +#ifndef _ASM_X86_UMIP_H
> > +#define _ASM_X86_UMIP_H
> > +
> > +#include
> > +#include
> > +
> > +#ifdef CONFIG_X86_INTEL_UMIP
> > +bool fixup_umip_exception(struct pt_regs *regs);
> > +#else
> > +static inline bool fixup_umip_exception(struct pt_regs *regs)
> > +{
> > + return false;
> > +}
>
> Let's save some header lines:
>
> static inline bool fixup_umip_exception(struct pt_regs *regs) {
> return false; }
>
> those trunks take too much space as it is.
I will correct.
>
> > +#endif /* CONFIG_X86_INTEL_UMIP */
> > +#endif /* _ASM_X86_UMIP_H */
> > diff --git a/arch/x86/kernel/Makefile b/arch/x86/kernel/Makefile
> > index 4b99423..cc1b7cc 100644
> > --- a/arch/x86/kernel/Makefile
> > +++ b/arch/x86/kernel/Makefile
> > @@ -123,6 +123,7 @@ obj-$(CONFIG_EFI) += sysfb_efi.o
> > obj-$(CONFIG_PERF_EVENTS) += perf_regs.o
> > obj-$(CONFIG_TRACING) += tracepoint.o
> > obj-$(CONFIG_SCHED_MC_PRIO)+= itmt.o
> > +obj-$(CONFIG_X86_INTEL_UMIP) += umip.o
> >
> > ifdef CONFIG_FRAME_POINTER
