Message-ID: <CALCETrWkL_RumwxcTKaQC=kBKZE4P5u95=OpcPGEjCJfaN9wVA@mail.gmail.com>
Date:   Fri, 23 Dec 2016 18:11:35 -0800
From:   Andy Lutomirski <luto@...capital.net>
To:     Ricardo Neri <ricardo.neri-calderon@...ux.intel.com>
Cc:     Ingo Molnar <mingo@...hat.com>,
        Thomas Gleixner <tglx@...utronix.de>,
        Borislav Petkov <bp@...e.de>,
        Andy Lutomirski <luto@...nel.org>,
        Peter Zijlstra <peterz@...radead.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        X86 ML <x86@...nel.org>, linux-msdos@...r.kernel.org,
        wine-devel@...ehq.org, Andrew Morton <akpm@...ux-foundation.org>,
        "H . Peter Anvin" <hpa@...or.com>, Brian Gerst <brgerst@...il.com>,
        Chen Yucong <slaoub@...il.com>,
        Chris Metcalf <cmetcalf@...lanox.com>,
        Dave Hansen <dave.hansen@...ux.intel.com>,
        Fenghua Yu <fenghua.yu@...el.com>,
        Huang Rui <ray.huang@....com>, Jiri Slaby <jslaby@...e.cz>,
        Jonathan Corbet <corbet@....net>,
        "Michael S . Tsirkin" <mst@...hat.com>,
        Paul Gortmaker <paul.gortmaker@...driver.com>,
        "Ravi V . Shankar" <ravi.v.shankar@...el.com>,
        Shuah Khan <shuah@...nel.org>,
        Vlastimil Babka <vbabka@...e.cz>,
        Tony Luck <tony.luck@...el.com>,
        Paolo Bonzini <pbonzini@...hat.com>,
        "Liang Z . Li" <liang.z.li@...el.com>,
        Alexandre Julliard <julliard@...ehq.org>,
        Stas Sergeev <stsp@...t.ru>
Subject: Re: [v2 5/7] x86: Add emulation code for UMIP instructions

On Fri, Dec 23, 2016 at 5:37 PM, Ricardo Neri
<ricardo.neri-calderon@...ux.intel.com> 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.
>
> 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) rely
> on this subset of instructions to function.
>
> The instructions protected by UMIP can be split in two groups. Those who
> return a kernel memory address (sgdt and sidt) and those who return a
> value (sldt, str and smsw).
>
> For the instructions that return a kernel memory address, the result is
> emulated as the location of a dummy variable in the kernel memory space.
> This is needed as applications such as WineHQ rely on the result being
> located in the kernel memory space function. The limit for the GDT and the
> IDT are set to zero.

Nak.  This is a trivial KASLR bypass.  Just give them hardcoded
values.  For x86_64, I would suggest 0xfffffffffffe0000 and
0xffffffffffff0000.

>
> The instructions sldt and str return a segment selector relative to the
> base address of the global descriptor table. Since the actual address of
> such table is not revealed, it makes sense to emulate the result as zero.

Hmm, now I wonder if anything uses SLDT to see if there is an LDT.  If
so, we could emulate it better, but I doubt this matters.

>
> The instruction smsw is emulated to return zero.

If you're going to emulate it, please return something plausible.  The
protected mode bit should be on, for example.  0x33 is probably
reasonable.

> +static int __emulate_umip_insn(struct insn *insn, enum umip_insn umip_inst,
> +                              unsigned char *data, int *data_size)
> +{
> +       unsigned long const *dummy_base_addr;
> +       unsigned short dummy_limit = 0;
> +       unsigned short dummy_value = 0;
> +
> +       switch (umip_inst) {
> +       /*
> +        * These two instructions return the base address and limit of the
> +        * global and interrupt descriptor table. The base address can be
> +        * 32-bit or 64-bit. Limit is always 16-bit.
> +        */
> +       case UMIP_SGDT:
> +       case UMIP_SIDT:
> +               if (umip_inst == UMIP_SGDT)
> +                       dummy_base_addr = &umip_dummy_gdt_base;
> +               else
> +                       dummy_base_addr = &umip_dummy_idt_base;
> +               if (X86_MODRM_MOD(insn->modrm.value) == 3) {
> +                       WARN_ONCE(1, "SGDT cannot take register as argument!\n");

No warnings please.

> +int fixup_umip_exception(struct pt_regs *regs)
> +{
> +       struct insn insn;
> +       unsigned char buf[MAX_INSN_SIZE];
> +       /* 10 bytes is the maximum size of the result of UMIP instructions */
> +       unsigned char dummy_data[10] = {0, 0, 0, 0, 0, 0, 0, 0, 0, 0};
> +       int x86_64 = !test_thread_flag(TIF_IA32);

user_64bit_mode(regs)

> +       int not_copied, nr_copied, reg_offset, dummy_data_size;
> +       void __user *uaddr;
> +       unsigned long *reg_addr;
> +       enum umip_insn umip_inst;
> +
> +       not_copied = copy_from_user(buf, (void __user *)regs->ip, sizeof(buf));

This is slightly wrong due to PKRU.  I doubt we care.

> +       nr_copied = sizeof(buf) - not_copied;
> +       /*
> +        * The decoder _should_ fail nicely if we pass it a short buffer.
> +        * But, let's not depend on that implementation detail.  If we
> +        * did not get anything, just error out now.
> +        */
> +       if (!nr_copied)
> +               return -EFAULT;

If the caller cares about EINVAL vs EFAULT, it cares because it is
considering changing the signal to a fake page fault.  If so, then
this should be EINVAL -- failure to read the text should just prevent
emulation.

> +       insn_init(&insn, buf, nr_copied, x86_64);
> +       insn_get_length(&insn);
> +       if (nr_copied < insn.length)
> +               return -EFAULT;

Ditto.

> +
> +       umip_inst = __identify_insn(&insn);
> +       /* Check if we found an instruction protected by UMIP */
> +       if (umip_inst < 0)
> +               return -EINVAL;
> +
> +       if (__emulate_umip_insn(&insn, umip_inst, dummy_data, &dummy_data_size))
> +               return -EINVAL;
> +
> +       /* If operand is a register, write directly to it */
> +       if (X86_MODRM_MOD(insn.modrm.value) == 3) {
> +               reg_offset = get_reg_offset_rm(&insn, regs);
> +               reg_addr = (unsigned long *)((unsigned long)regs + reg_offset);
> +               memcpy(reg_addr, dummy_data, dummy_data_size);
> +       } else {
> +               uaddr = insn_get_addr_ref(&insn, regs);
> +               nr_copied = copy_to_user(uaddr, dummy_data, dummy_data_size);
> +               if (nr_copied  > 0)
> +                       return -EFAULT;

This should be the only EFAULT case.

Powered by blists - more mailing lists