lkml.org 
[lkml]   [2016]   [Dec]   [27]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    Date
    SubjectRe: [v2 5/7] x86: Add emulation code for UMIP instructions
    On Tue, Dec 27, 2016 at 4:39 PM, Ricardo Neri
    <ricardo.neri-calderon@linux.intel.com> wrote:
    > On Fri, 2016-12-23 at 18:11 -0800, Andy Lutomirski wrote:
    >> On Fri, Dec 23, 2016 at 5:37 PM, Ricardo Neri
    >> <ricardo.neri-calderon@linux.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.
    >
    > I see. I assume you are suggesting these values for x86_64 because they
    > lie in an unused hole. That makes sense to me.
    >
    > For the case of x86_32, I have trouble finding a suitable place as there
    > are not many available holes. It could be put before VMALLOC_START or
    > after VMALLOC_END but this would reveal the position of the vmalloc
    > area. Although, to my knowledge, randomized memory is not available for
    > x86_32. Without randomization, does it hurt to make sidt/sgdt return the
    > address of a kernel static variable?

    I would just use the same addresses, truncated. There's no reason
    that the address needs to be truly not present -- it just needs to be
    inaccessible to user code. Anything near the top of the address space
    should work.

    >
    >>
    >> >
    >> > 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.
    >
    > So you are saying that the emulated sldt should return a different value
    > based on the presence/absence of a LDT? This could reveal this very
    > fact.

    User code knows whether the LDT exists because an LDT only exists if
    the program called modify_ldt(). But I doubt this matters in
    practice.

    >> > +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.
    >
    > I'll. Remove it.

    Thanks. In general, WARN_ONCE, etc are supposed to indicate kernel
    bugs, not user bugs.

    >> > + 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.
    >
    > I see. If I am not mistaken, if the memory is protected by a protection
    > key this would cause a page fault. I'll make a note of it.

    Exactly. This is correct behavior unless the key happens to be set up
    so it can be executed but not read, in which case emulation will fail.

    >>
    >> > + 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.
    >
    > I see. The caller in this case do_general_protection, which will issue a
    > SIGSEGV to the user space anyways. I don't think it cares about the
    > EINVAL vs EFAULT. It does care about whether the emulation was
    > successful.

    Maybe just make it return bool then? But fixing up the return codes
    would be fine, too. I just think that, if it returns int, the value
    should be meaningful.

    >> > + if (nr_copied > 0)
    >> > + return -EFAULT;
    >>
    >> This should be the only EFAULT case.
    > Should this be EFAULT event if the caller cares only about successful
    > (return 0) vs failed (return non-0) emulation?

    In theory this particular error would be a page fault not a general
    protection fault (in the UMIP off case). If you were emulating it
    extra carefully, you could change the signal accordingly. But, as I
    said, I really doubt this matters.

    \
     
     \ /
      Last update: 2016-12-28 01:49    [W:4.533 / U:0.944 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site