December 2016 - Wine-Devel - list.winehq.org

Re: [PATCH 2/2] regedit: Only allow value renaming via the Edit or Popup menus

by Hugh McMaster

On Thursday, 29 December 2016 1:10 AM, Ken Thomases wrote: > I personally dislike this proposed change. I use click-to-rename values all the time and can't think of why that would be a problem. This behaviour isn't available in the Windows version of regedit, so that's my primary motivation for altering the renaming behaviour. Personally, I just don't find the click to rename behaviour that useful. I guess that's because I'm so used to how the Windows version behaves. In Wine, right-clicking on a value activates the pop-up menu. Left-clicking out of that menu on the same entry activates renaming mode. I find that behaviour unexpected and annoying. But that's just me. Thanks for the feedback. Hugh

Dec. 29, 2016

Re: [PATCH 2/2] regedit: Only allow value renaming via the Edit or Popup menus

by Hugh McMaster

On Wednesday, 28 December 2016 11:47 PM, Nikolay Sivov wrote: > On 28.12.2016 14:47, Hugh McMaster wrote: >> @@ -485,7 +494,7 @@ HWND CreateListView(HWND hwndParent, UINT id) >> /* Get the dimensions of the parent window's client area, and create the list view control. */ >> GetClientRect(hwndParent, &rcClient); >> hwndLV = CreateWindowExW(WS_EX_CLIENTEDGE, WC_LISTVIEWW, ListView, >> - WS_VISIBLE | WS_CHILD | WS_TABSTOP | LVS_REPORT | LVS_EDITLABELS, >> + WS_VISIBLE | WS_CHILD | WS_TABSTOP | LVS_REPORT, >> 0, 0, rcClient.right, rcClient.bottom, >> hwndParent, ULongToHandle(id), hInst, NULL); >> if (!hwndLV) return NULL; > Clean way to do that is to block edit operation using LVN_BEGINLABELEDIT > notification. So basically you should always return TRUE from it, except > for the case when edit is triggered with UI command, in this case you > can temporarily enable edit, and block it back when done. But I'm > curious, why do you want to do that in a first place, what's wrong with > the ability to click-rename items? Patch 1/2 prevents label renaming via left-click in regedit's treeview controls. (See also [1]). The same left-click to rename behaviour also occurs with regedit's ListView control, so I'm just extending the same solution to it, so we don't have different behaviour. And as noted in [1], you can't click to rename in the Windows version of regedit. I thought of using LVN_BEGINLABELEDIT to handle the edit behaviour, but I'm not sure how to determine whether the command was sent from the UI via WM_COMMAND or directly from the ListView click via WM_NOTIFY. [1] https://bugs.winehq.org/show_bug.cgi?id=37458

Dec. 29, 2016

Re: [PATCH 2/2] regedit: Only allow value renaming via the Edit or Popup menus

by Ken Thomases

Hi, I personally dislike this proposed change. I use click-to-rename values all the time and can't think of why that would be a problem. -Ken

Dec. 28, 2016

Re: [PATCH] winmm: RegQueryValueExW wants the size in bytes, not the size in characters.

by Andrew Eikum

Thanks. These look good but we're in code freeze. I'll be happy to sign-off on them after the freeze is over. Andrew On Mon, Dec 26, 2016 at 04:25:22PM +0100, Carlo Bramini wrote: > There is a bug into dlls/winmm/playsound.c > Function RegQueryValueExW accepts the size in bytes as parameter, not the size > in characters. > > See: > https://msdn.microsoft.com/en-us/library/windows/desktop/ms724911(v=vs.85). > aspx > > Sincerely. > > Signed-off-by: Carlo Bramini <carlo_bramini(a)users.sourceforge.net> > > diff --git a/dlls/winmm/playsound.c b/dlls/winmm/playsound.c > index 5d9aed0..6525db5 100644 > --- a/dlls/winmm/playsound.c > +++ b/dlls/winmm/playsound.c > @@ -140,7 +140,7 @@ static HMMIO get_mmioFromProfile(UINT uFlags, LPCWSTR lpszName) > if (err != 0) > goto none; > } > - count = sizeof(str)/sizeof(str[0]); > + count = sizeof(str); > err = RegQueryValueExW(hSnd, NULL, 0, &type, (LPBYTE)str, &count); > RegCloseKey(hSnd); > if (err != 0 || !*str) goto none; >

Dec. 28, 2016

Re: [PATCH 2/2] regedit: Only allow value renaming via the Edit or Popup menus

by Nikolay Sivov

On 28.12.2016 14:47, Hugh McMaster wrote: > @@ -485,7 +494,7 @@ HWND CreateListView(HWND hwndParent, UINT id) > /* Get the dimensions of the parent window's client area, and create the list view control. */ > GetClientRect(hwndParent, &rcClient); > hwndLV = CreateWindowExW(WS_EX_CLIENTEDGE, WC_LISTVIEWW, ListView, > - WS_VISIBLE | WS_CHILD | WS_TABSTOP | LVS_REPORT | LVS_EDITLABELS, > + WS_VISIBLE | WS_CHILD | WS_TABSTOP | LVS_REPORT, > 0, 0, rcClient.right, rcClient.bottom, > hwndParent, ULongToHandle(id), hInst, NULL); > if (!hwndLV) return NULL; Clean way to do that is to block edit operation using LVN_BEGINLABELEDIT notification. So basically you should always return TRUE from it, except for the case when edit is triggered with UI command, in this case you can temporarily enable edit, and block it back when done. But I'm curious, why do you want to do that in a first place, what's wrong with the ability to click-rename items?

Dec. 28, 2016

Re: [v2 5/7] x86: Add emulation code for UMIP instructions

by Andy Lutomirski

On Tue, Dec 27, 2016 at 4:39 PM, Ricardo Neri <ricardo.neri-calderon(a)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(a)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.

Dec. 28, 2016

Re: [v2 5/7] x86: Add emulation code for UMIP instructions

by Ricardo Neri

On Mon, 2016-12-26 at 00:49 +0900, Masami Hiramatsu wrote: > On Fri, 23 Dec 2016 17:37:43 -0800 > Ricardo Neri <ricardo.neri-calderon(a)linux.intel.com> wrote: > > > +static int __identify_insn(struct insn *insn) > > +{ > > + /* by getting modrm we also get the opcode */ > > + insn_get_modrm(insn); > > + if (insn->opcode.bytes[0] != 0xf) > > + return -EINVAL; > > + > > + if (insn->opcode.bytes[1] == 0x1) { > > + switch (X86_MODRM_REG(insn->modrm.value)) { > > + case 0: > > + return UMIP_SGDT; > > + case 1: > > + return UMIP_SIDT; > > + case 4: > > + return UMIP_SMSW; > > + default: > > + return -EINVAL; > > + } > > + } else if (insn->opcode.bytes[1] == 0x0) { > > + if (X86_MODRM_REG(insn->modrm.value) == 0) > > + return UMIP_SLDT; > > + else if (X86_MODRM_REG(insn->modrm.value) == 1) > > + return UMIP_STR; > > + else > > + return -EINVAL; > > + } > > gcc detected an error here, you may need return "-EINVAL". I will make this change. I removed this EINVAL at the last minute as it didn't look right. It was indeed right. Thanks and BR, Ricardo > > Thanks, > > >

Dec. 28, 2016

Re: [v2 5/7] x86: Add emulation code for UMIP instructions

by Ricardo Neri

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(a)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? > > > > > 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. > > > > > 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. Sure. Will do. > > > +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. > > > +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) I'll make this change. > > > + 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. > > > + 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. > > > + insn_init(&insn, buf, nr_copied, x86_64); > > + insn_get_length(&insn); > > + if (nr_copied < insn.length) > > + return -EFAULT; > > Ditto. I will change to EINVAL. > > > + > > + 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. Should this be EFAULT event if the caller cares only about successful (return 0) vs failed (return non-0) emulation? Thanks for your thorough review! I really appreciate it. Thanks and BR, Ricardo

Dec. 28, 2016

Re: [v2 3/7] x86/mpx, x86/insn: Relocate insn util functions to a new insn-utils

by Ricardo Neri

On Sun, 2016-12-25 at 15:17 +0900, Masami Hiramatsu wrote: > Hi Ricado, > > On Fri, 23 Dec 2016 17:37:41 -0800 > Ricardo Neri <ricardo.neri-calderon(a)linux.intel.com> wrote: > > > Other kernel submodules can benefit from using the utility functions > > defined in mpx.c to obtain the addresses and values of operands contained > > in the general purpose registers. An instance of this is the emulation code > > used for instructions protected by the Intel User-Mode Instruction > > Prevention feature. > > > > Thus, these functions are relocated to a new insn-utils.c file. The reason > > to not relocate these utilities for insn.c is that the latter solely > > analyses instructions given by a struct insn. The file insn-utils.c intends > > to be used when, for instance, determining addresses from the contents > > of the general purpose registers. > > > > To avoid creating a new insn-utils.h, insn.h is used. One caveat, however, > > is that several #include's were needed by the utility functions. > > > > Functions are simply relocated. There are not functional or indentation > > changes. > > Thank you for your great work! :) Thanks a lot for taking the time to review! > > > --- > > arch/x86/include/asm/insn.h | 6 ++ > > arch/x86/lib/Makefile | 2 +- > > arch/x86/lib/insn-utils.c | 148 ++++++++++++++++++++++++++++++++++++++++++++ > > arch/x86/mm/mpx.c | 136 +--------------------------------------- > > 4 files changed, 156 insertions(+), 136 deletions(-) > > create mode 100644 arch/x86/lib/insn-utils.c > > > > diff --git a/arch/x86/include/asm/insn.h b/arch/x86/include/asm/insn.h > > index b3e32b0..9dc9d42 100644 > > --- a/arch/x86/include/asm/insn.h > > +++ b/arch/x86/include/asm/insn.h > > @@ -22,6 +22,10 @@ > > > > /* insn_attr_t is defined in inat.h */ > > #include <asm/inat.h> > > +#include <linux/compiler.h> > > +#include <linux/bug.h> > > +#include <linux/err.h> > > +#include <asm/ptrace.h> > > > > struct insn_field { > > union { > > @@ -106,6 +110,8 @@ extern void insn_get_sib(struct insn *insn); > > extern void insn_get_displacement(struct insn *insn); > > extern void insn_get_immediate(struct insn *insn); > > extern void insn_get_length(struct insn *insn); > > +extern void __user *insn_get_addr_ref(struct insn *insn, struct pt_regs *regs); > > +extern int get_reg_offset_rm(struct insn *insn, struct pt_regs *regs); > > Could you also rename this to add "insn_" prefix? Sure. This should have the prefix. I missed this. > > Other part looks good to me :) > (btw, I saw a kbuild bot warning, would you also test it with > CONFIG_X86_DECODER_SELFTEST=y?) I will do. Thanks and BR, Ricardo > > Thanks again! > > > > > /* Attribute will be determined after getting ModRM (for opcode groups) */ > > static inline void insn_get_attribute(struct insn *insn) > > diff --git a/arch/x86/lib/Makefile b/arch/x86/lib/Makefile > > index 34a7413..0d01d82 100644 > > --- a/arch/x86/lib/Makefile > > +++ b/arch/x86/lib/Makefile > > @@ -23,7 +23,7 @@ lib-y := delay.o misc.o cmdline.o cpu.o > > lib-y += usercopy_$(BITS).o usercopy.o getuser.o putuser.o > > lib-y += memcpy_$(BITS).o > > lib-$(CONFIG_RWSEM_XCHGADD_ALGORITHM) += rwsem.o > > -lib-$(CONFIG_INSTRUCTION_DECODER) += insn.o inat.o > > +lib-$(CONFIG_INSTRUCTION_DECODER) += insn.o inat.o insn-utils.o > > lib-$(CONFIG_RANDOMIZE_BASE) += kaslr.o > > > > obj-y += msr.o msr-reg.o msr-reg-export.o hweight.o > > diff --git a/arch/x86/lib/insn-utils.c b/arch/x86/lib/insn-utils.c > > new file mode 100644 > > index 0000000..598bbd6 > > --- /dev/null > > +++ b/arch/x86/lib/insn-utils.c > > @@ -0,0 +1,148 @@ > > +/* > > + * Utility functions for x86 operand and address decoding > > + * > > + * Copyright (C) Intel Corporation 2016 > > + */ > > +#include <linux/kernel.h> > > +#include <linux/string.h> > > +#include <asm/inat.h> > > +#include <asm/insn.h> > > + > > +enum reg_type { > > + REG_TYPE_RM = 0, > > + REG_TYPE_INDEX, > > + REG_TYPE_BASE, > > +}; > > + > > +static int get_reg_offset(struct insn *insn, struct pt_regs *regs, > > + enum reg_type type) > > +{ > > + int regno = 0; > > + > > + static const int regoff[] = { > > + offsetof(struct pt_regs, ax), > > + offsetof(struct pt_regs, cx), > > + offsetof(struct pt_regs, dx), > > + offsetof(struct pt_regs, bx), > > + offsetof(struct pt_regs, sp), > > + offsetof(struct pt_regs, bp), > > + offsetof(struct pt_regs, si), > > + offsetof(struct pt_regs, di), > > +#ifdef CONFIG_X86_64 > > + offsetof(struct pt_regs, r8), > > + offsetof(struct pt_regs, r9), > > + offsetof(struct pt_regs, r10), > > + offsetof(struct pt_regs, r11), > > + offsetof(struct pt_regs, r12), > > + offsetof(struct pt_regs, r13), > > + offsetof(struct pt_regs, r14), > > + offsetof(struct pt_regs, r15), > > +#endif > > + }; > > + int nr_registers = ARRAY_SIZE(regoff); > > + /* > > + * Don't possibly decode a 32-bit instructions as > > + * reading a 64-bit-only register. > > + */ > > + if (IS_ENABLED(CONFIG_X86_64) && !insn->x86_64) > > + nr_registers -= 8; > > + > > + switch (type) { > > + case REG_TYPE_RM: > > + regno = X86_MODRM_RM(insn->modrm.value); > > + if (X86_REX_B(insn->rex_prefix.value)) > > + regno += 8; > > + break; > > + > > + case REG_TYPE_INDEX: > > + regno = X86_SIB_INDEX(insn->sib.value); > > + if (X86_REX_X(insn->rex_prefix.value)) > > + regno += 8; > > + /* > > + * If mod !=3, SP is not used as index. Check is done after > > + * looking at REX.X This is because R12 can be used as an > > + * index. > > + */ > > + if (regno == 4 && X86_MODRM_RM(insn->modrm.value) != 3) > > + return 0; > > + break; > > + > > + case REG_TYPE_BASE: > > + regno = X86_SIB_BASE(insn->sib.value); > > + if (regno == 5 && X86_MODRM_RM(insn->modrm.value) == 0) { > > + WARN_ONCE(1, "An explicit displacement is required when %sBP used as SIB base.", > > + (IS_ENABLED(CONFIG_X86_64) && insn->x86_64) ? > > + "R13 or R" : "E"); > > + return -EINVAL; > > + } > > + > > + if (X86_REX_B(insn->rex_prefix.value)) > > + regno += 8; > > + break; > > + > > + default: > > + pr_err("invalid register type"); > > + BUG(); > > + break; > > + } > > + > > + if (regno >= nr_registers) { > > + WARN_ONCE(1, "decoded an instruction with an invalid register"); > > + return -EINVAL; > > + } > > + return regoff[regno]; > > +} > > + > > +int get_reg_offset_rm(struct insn *insn, struct pt_regs *regs) > > +{ > > + return get_reg_offset(insn, regs, REG_TYPE_RM); > > +} > > + > > +/* > > + * return the address being referenced be instruction > > + * for rm=3 returning the content of the rm reg > > + * for rm!=3 calculates the address using SIB and Disp > > + */ > > +void __user *insn_get_addr_ref(struct insn *insn, struct pt_regs *regs) > > +{ > > + unsigned long addr, base, indx; > > + int addr_offset, base_offset, indx_offset; > > + insn_byte_t sib; > > + > > + insn_get_modrm(insn); > > + insn_get_sib(insn); > > + sib = insn->sib.value; > > + > > + if (X86_MODRM_MOD(insn->modrm.value) == 3) { > > + addr_offset = get_reg_offset(insn, regs, REG_TYPE_RM); > > + if (addr_offset < 0) > > + goto out_err; > > + addr = regs_get_register(regs, addr_offset); > > + } else { > > + if (insn->sib.nbytes) { > > + base_offset = get_reg_offset(insn, regs, REG_TYPE_BASE); > > + if (base_offset < 0) > > + goto out_err; > > + > > + indx_offset = get_reg_offset(insn, regs, REG_TYPE_INDEX); > > + if (indx_offset < 0) > > + goto out_err; > > + > > + base = regs_get_register(regs, base_offset); > > + if (indx_offset) > > + indx = regs_get_register(regs, indx_offset); > > + else > > + indx = 0; > > + addr = base + indx * (1 << X86_SIB_SCALE(sib)); > > + } else { > > + addr_offset = get_reg_offset(insn, regs, REG_TYPE_RM); > > + if (addr_offset < 0) > > + goto out_err; > > + addr = regs_get_register(regs, addr_offset); > > + } > > + addr += insn->displacement.value; > > + } > > + return (void __user *)addr; > > +out_err: > > + return (void __user *)-1; > > +} > > diff --git a/arch/x86/mm/mpx.c b/arch/x86/mm/mpx.c > > index 71681d0..32ba342 100644 > > --- a/arch/x86/mm/mpx.c > > +++ b/arch/x86/mm/mpx.c > > @@ -59,140 +59,6 @@ static unsigned long mpx_mmap(unsigned long len) > > return addr; > > } > > > > -enum reg_type { > > - REG_TYPE_RM = 0, > > - REG_TYPE_INDEX, > > - REG_TYPE_BASE, > > -}; > > - > > -static int get_reg_offset(struct insn *insn, struct pt_regs *regs, > > - enum reg_type type) > > -{ > > - int regno = 0; > > - > > - static const int regoff[] = { > > - offsetof(struct pt_regs, ax), > > - offsetof(struct pt_regs, cx), > > - offsetof(struct pt_regs, dx), > > - offsetof(struct pt_regs, bx), > > - offsetof(struct pt_regs, sp), > > - offsetof(struct pt_regs, bp), > > - offsetof(struct pt_regs, si), > > - offsetof(struct pt_regs, di), > > -#ifdef CONFIG_X86_64 > > - offsetof(struct pt_regs, r8), > > - offsetof(struct pt_regs, r9), > > - offsetof(struct pt_regs, r10), > > - offsetof(struct pt_regs, r11), > > - offsetof(struct pt_regs, r12), > > - offsetof(struct pt_regs, r13), > > - offsetof(struct pt_regs, r14), > > - offsetof(struct pt_regs, r15), > > -#endif > > - }; > > - int nr_registers = ARRAY_SIZE(regoff); > > - /* > > - * Don't possibly decode a 32-bit instructions as > > - * reading a 64-bit-only register. > > - */ > > - if (IS_ENABLED(CONFIG_X86_64) && !insn->x86_64) > > - nr_registers -= 8; > > - > > - switch (type) { > > - case REG_TYPE_RM: > > - regno = X86_MODRM_RM(insn->modrm.value); > > - if (X86_REX_B(insn->rex_prefix.value)) > > - regno += 8; > > - break; > > - > > - case REG_TYPE_INDEX: > > - regno = X86_SIB_INDEX(insn->sib.value); > > - if (X86_REX_X(insn->rex_prefix.value)) > > - regno += 8; > > - /* > > - * If mod !=3, SP is not used as index. Check is done after > > - * looking at REX.X This is because R12 can be used as an > > - * index. > > - */ > > - if (regno == 4 && X86_MODRM_RM(insn->modrm.value) != 3) > > - return 0; > > - break; > > - > > - case REG_TYPE_BASE: > > - regno = X86_SIB_BASE(insn->sib.value); > > - if (regno == 5 && X86_MODRM_RM(insn->modrm.value) == 0) { > > - WARN_ONCE(1, "An explicit displacement is required when %sBP used as SIB base.", > > - (IS_ENABLED(CONFIG_X86_64) && insn->x86_64) ? > > - "R13 or R" : "E"); > > - return -EINVAL; > > - } > > - > > - if (X86_REX_B(insn->rex_prefix.value)) > > - regno += 8; > > - break; > > - > > - default: > > - pr_err("invalid register type"); > > - BUG(); > > - break; > > - } > > - > > - if (regno >= nr_registers) { > > - WARN_ONCE(1, "decoded an instruction with an invalid register"); > > - return -EINVAL; > > - } > > - return regoff[regno]; > > -} > > - > > -/* > > - * return the address being referenced be instruction > > - * for rm=3 returning the content of the rm reg > > - * for rm!=3 calculates the address using SIB and Disp > > - */ > > -static void __user *mpx_get_addr_ref(struct insn *insn, struct pt_regs *regs) > > -{ > > - unsigned long addr, base, indx; > > - int addr_offset, base_offset, indx_offset; > > - insn_byte_t sib; > > - > > - insn_get_modrm(insn); > > - insn_get_sib(insn); > > - sib = insn->sib.value; > > - > > - if (X86_MODRM_MOD(insn->modrm.value) == 3) { > > - addr_offset = get_reg_offset(insn, regs, REG_TYPE_RM); > > - if (addr_offset < 0) > > - goto out_err; > > - addr = regs_get_register(regs, addr_offset); > > - } else { > > - if (insn->sib.nbytes) { > > - base_offset = get_reg_offset(insn, regs, REG_TYPE_BASE); > > - if (base_offset < 0) > > - goto out_err; > > - > > - indx_offset = get_reg_offset(insn, regs, REG_TYPE_INDEX); > > - if (indx_offset < 0) > > - goto out_err; > > - > > - base = regs_get_register(regs, base_offset); > > - if (indx_offset) > > - indx = regs_get_register(regs, indx_offset); > > - else > > - indx = 0; > > - addr = base + indx * (1 << X86_SIB_SCALE(sib)); > > - } else { > > - addr_offset = get_reg_offset(insn, regs, REG_TYPE_RM); > > - if (addr_offset < 0) > > - goto out_err; > > - addr = regs_get_register(regs, addr_offset); > > - } > > - addr += insn->displacement.value; > > - } > > - return (void __user *)addr; > > -out_err: > > - return (void __user *)-1; > > -} > > - > > static int mpx_insn_decode(struct insn *insn, > > struct pt_regs *regs) > > { > > @@ -305,7 +171,7 @@ siginfo_t *mpx_generate_siginfo(struct pt_regs *regs) > > info->si_signo = SIGSEGV; > > info->si_errno = 0; > > info->si_code = SEGV_BNDERR; > > - info->si_addr = mpx_get_addr_ref(&insn, regs); > > + info->si_addr = insn_get_addr_ref(&insn, regs); > > /* > > * We were not able to extract an address from the instruction, > > * probably because there was something invalid in it. > > -- > > 2.9.3 > > > >

Dec. 27, 2016

Re: [v2 6/7] x86/traps: Fixup general protection faults caused by UMIP

by Ricardo Neri

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(a)linux.intel.com> wrote: > > If the User-Mode Instruction Prevention CPU feature is available and > > enabled, a general protection fault will be issued if the instructions > > sgdt, sldt, sidt, str or smsw are executed from user-mode context > > (CPL > 0). If the fault was caused by any of the instructions protected > > by UMIP, fixup_umip_exceptino will emulate dummy results for these > > instructions. > > > > Cc: Andy Lutomirski <luto(a)kernel.org> > > Cc: Andrew Morton <akpm(a)linux-foundation.org> > > Cc: H. Peter Anvin <hpa(a)zytor.com> > > Cc: Borislav Petkov <bp(a)suse.de> > > Cc: Brian Gerst <brgerst(a)gmail.com> > > Cc: Chen Yucong <slaoub(a)gmail.com> > > Cc: Chris Metcalf <cmetcalf(a)mellanox.com> > > Cc: Dave Hansen <dave.hansen(a)linux.intel.com> > > Cc: Fenghua Yu <fenghua.yu(a)intel.com> > > Cc: Huang Rui <ray.huang(a)amd.com> > > Cc: Jiri Slaby <jslaby(a)suse.cz> > > Cc: Jonathan Corbet <corbet(a)lwn.net> > > Cc: Michael S. Tsirkin <mst(a)redhat.com> > > Cc: Paul Gortmaker <paul.gortmaker(a)windriver.com> > > Cc: Peter Zijlstra <peterz(a)infradead.org> > > Cc: Ravi V. Shankar <ravi.v.shankar(a)intel.com> > > Cc: Shuah Khan <shuah(a)kernel.org> > > Cc: Vlastimil Babka <vbabka(a)suse.cz> > > Cc: Tony Luck <tony.luck(a)intel.com> > > Cc: Paolo Bonzini <pbonzini(a)redhat.com> > > Cc: Liang Z. Li <liang.z.li(a)intel.com> > > Cc: Alexandre Julliard <julliard(a)winehq.org> > > Cc: Stas Sergeev <stsp(a)list.ru> > > Cc: x86(a)kernel.org > > Cc: linux-msdos(a)vger.kernel.org > > Signed-off-by: Ricardo Neri <ricardo.neri-calderon(a)linux.intel.com> > > --- > > arch/x86/kernel/traps.c | 4 ++++ > > 1 file changed, 4 insertions(+) > > > > diff --git a/arch/x86/kernel/traps.c b/arch/x86/kernel/traps.c > > index bf0c6d0..5044fb3 100644 > > --- a/arch/x86/kernel/traps.c > > +++ b/arch/x86/kernel/traps.c > > @@ -64,6 +64,7 @@ > > #include <asm/trace/mpx.h> > > #include <asm/mpx.h> > > #include <asm/vm86.h> > > +#include <asm/umip.h> > > > > #ifdef CONFIG_X86_64 > > #include <asm/x86_init.h> > > @@ -491,6 +492,9 @@ do_general_protection(struct pt_regs *regs, long error_code) > > RCU_LOCKDEP_WARN(!rcu_is_watching(), "entry code didn't wake RCU"); > > cond_local_irq_enable(regs); > > > > + if (user_mode(regs) && !fixup_umip_exception(regs)) > > + return; > > + > > I would do fixup_umip_exception(regs) == 0 to make it more obvious > what's going on. Sure. I will make this change. > > Also, since you're allowing this in v8086 mode, I think this should > have an explicit test in > tools/testing/selftests/x86/entry_from_vm86.c. I *think* it will work > fine, but the pt_regs handling in vm86 mode is quite scary and has > been rewritten recently. I will include a test for this. Thanks and BR, Ricardo

Dec. 27, 2016