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

Emitting aligned SSE movs in gcc with force_align_arg_pointer

by Daniel Santos

I'm working on a few improvements in GCC targeting 64-bit Wine. I've got a patch set that gives us back aligned SSE movs for 64-bit ms_abi functions that call System V functions. However, it looks like I'll have to wait until GCC 8's stage 1 comes around (https://gcc.gnu.org/ml/gcc-patches/2016-12/msg01924.html) but I thought I should post this here to prevent any duplication of effort. (Come to think of it, I should probably open a gcc bug report for this.) Following this patch set I have the out-of-lining patch set for the msabi/sysv prologues & epilogues as well that I'm still refining and cleaning up a bit. Happy New Year all! Daniel

Dec. 31, 2016

Re: [2/3] ntdll: Implement FileIdInformation class support in NtQueryInformationFile.

by Jon Doron

I was referring to ntifs.h because this has to do with ntdll, do you still want me to move it to winbase? this will probably cause compilation errors in the NTDLL part that implements the functionality On Sat, Dec 31, 2016 at 4:04 PM, Nikolay Sivov <nsivov(a)codeweavers.com> wrote: > On 31.12.2016 16:50, Jonathan Doron wrote: >> Signed-off-by: Jonathan Doron <jond(a)wizery.com> >> --- >> dlls/ntdll/file.c | 19 ++++++++++++++++++- >> include/winternl.h | 9 +++++++++ >> 2 files changed, 27 insertions(+), 1 deletion(-) >> >> diff --git a/dlls/ntdll/file.c b/dlls/ntdll/file.c >> index 6eddd19..fbadfbd 100644 >> --- a/dlls/ntdll/file.c >> +++ b/dlls/ntdll/file.c >> @@ -2379,7 +2379,7 @@ NTSTATUS WINAPI NtQueryInformationFile( HANDLE hFile, PIO_STATUS_BLOCK io, >> 0, /* FileRenameInformationBypassAccessCheck */ >> 0, /* FileLinkInformationBypassAccessCheck */ >> 0, /* FileVolumeNameInformation */ >> - 0, /* FileIdInformation */ >> + sizeof(FILE_ID_INFORMATION), /* FileIdInformation */ >> 0, /* FileIdExtdDirectoryInformation */ >> 0, /* FileReplaceCompletionInformation */ >> 0, /* FileHardLinkFullIdInformation */ >> @@ -2621,6 +2621,23 @@ NTSTATUS WINAPI NtQueryInformationFile( HANDLE hFile, PIO_STATUS_BLOCK io, >> } >> } >> break; >> + case FileIdInformation: >> + { >> + struct statfs volstats; >> + FILE_ID_INFORMATION *info = ptr; >> + if ((fstat(fd, &st) == -1) || (fstatfs(fd, &volstats) == -1)) >> + { >> + io->u.Status = FILE_GetNtStatus(); >> + } >> + else >> + { >> + memset(info, 0, sizeof(*info)); >> + ((PLARGE_INTEGER)&info->VolumeSerialNumber)->LowPart = (ULONG)volstats.f_fsid.__val[0]; >> + ((PLARGE_INTEGER)&info->VolumeSerialNumber)->HighPart = (ULONG)volstats.f_fsid.__val[1]; >> + ((PLARGE_INTEGER)&info->FileId)->QuadPart = st.st_ino; >> + } >> + } >> + break; >> default: >> FIXME("Unsupported class (%d)\n", class); >> io->u.Status = STATUS_NOT_IMPLEMENTED; >> diff --git a/include/winternl.h b/include/winternl.h >> index 891b6a7..2f2ea86 100644 >> --- a/include/winternl.h >> +++ b/include/winternl.h >> @@ -571,6 +571,15 @@ typedef struct _FILE_INTERNAL_INFORMATION { >> LARGE_INTEGER IndexNumber; >> } FILE_INTERNAL_INFORMATION, *PFILE_INTERNAL_INFORMATION; >> >> +typedef struct _FILE_ID_128 { >> + UCHAR Identifier[16]; >> +} FILE_ID_128, *PFILE_ID_128; > Windows SDK has it in winnt.h, using BYTE instead of UCHAR. > >> + >> +typedef struct _FILE_ID_INFORMATION { >> + ULONGLONG VolumeSerialNumber; >> + FILE_ID_128 FileId; >> +} FILE_ID_INFORMATION, *PFILE_ID_INFORMATION; >> + > This one should go to winbase.h, as FILE_ID_INFO. >> typedef struct _FILE_EA_INFORMATION { >> ULONG EaSize; >> } FILE_EA_INFORMATION, *PFILE_EA_INFORMATION; > >

Dec. 31, 2016

Re: [2/3] ntdll: Implement FileIdInformation class support in NtQueryInformationFile.

by Nikolay Sivov

On 31.12.2016 16:50, Jonathan Doron wrote: > Signed-off-by: Jonathan Doron <jond(a)wizery.com> > --- > dlls/ntdll/file.c | 19 ++++++++++++++++++- > include/winternl.h | 9 +++++++++ > 2 files changed, 27 insertions(+), 1 deletion(-) > > diff --git a/dlls/ntdll/file.c b/dlls/ntdll/file.c > index 6eddd19..fbadfbd 100644 > --- a/dlls/ntdll/file.c > +++ b/dlls/ntdll/file.c > @@ -2379,7 +2379,7 @@ NTSTATUS WINAPI NtQueryInformationFile( HANDLE hFile, PIO_STATUS_BLOCK io, > 0, /* FileRenameInformationBypassAccessCheck */ > 0, /* FileLinkInformationBypassAccessCheck */ > 0, /* FileVolumeNameInformation */ > - 0, /* FileIdInformation */ > + sizeof(FILE_ID_INFORMATION), /* FileIdInformation */ > 0, /* FileIdExtdDirectoryInformation */ > 0, /* FileReplaceCompletionInformation */ > 0, /* FileHardLinkFullIdInformation */ > @@ -2621,6 +2621,23 @@ NTSTATUS WINAPI NtQueryInformationFile( HANDLE hFile, PIO_STATUS_BLOCK io, > } > } > break; > + case FileIdInformation: > + { > + struct statfs volstats; > + FILE_ID_INFORMATION *info = ptr; > + if ((fstat(fd, &st) == -1) || (fstatfs(fd, &volstats) == -1)) > + { > + io->u.Status = FILE_GetNtStatus(); > + } > + else > + { > + memset(info, 0, sizeof(*info)); > + ((PLARGE_INTEGER)&info->VolumeSerialNumber)->LowPart = (ULONG)volstats.f_fsid.__val[0]; > + ((PLARGE_INTEGER)&info->VolumeSerialNumber)->HighPart = (ULONG)volstats.f_fsid.__val[1]; > + ((PLARGE_INTEGER)&info->FileId)->QuadPart = st.st_ino; > + } > + } > + break; > default: > FIXME("Unsupported class (%d)\n", class); > io->u.Status = STATUS_NOT_IMPLEMENTED; > diff --git a/include/winternl.h b/include/winternl.h > index 891b6a7..2f2ea86 100644 > --- a/include/winternl.h > +++ b/include/winternl.h > @@ -571,6 +571,15 @@ typedef struct _FILE_INTERNAL_INFORMATION { > LARGE_INTEGER IndexNumber; > } FILE_INTERNAL_INFORMATION, *PFILE_INTERNAL_INFORMATION; > > +typedef struct _FILE_ID_128 { > + UCHAR Identifier[16]; > +} FILE_ID_128, *PFILE_ID_128; Windows SDK has it in winnt.h, using BYTE instead of UCHAR. > + > +typedef struct _FILE_ID_INFORMATION { > + ULONGLONG VolumeSerialNumber; > + FILE_ID_128 FileId; > +} FILE_ID_INFORMATION, *PFILE_ID_INFORMATION; > + This one should go to winbase.h, as FILE_ID_INFO. > typedef struct _FILE_EA_INFORMATION { > ULONG EaSize; > } FILE_EA_INFORMATION, *PFILE_EA_INFORMATION;

Dec. 31, 2016

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

by Andy Lutomirski

On Thu, Dec 29, 2016 at 9:23 PM, Ricardo Neri <ricardo.neri-calderon(a)linux.intel.com> wrote: > On Tue, 2016-12-27 at 16:48 -0800, Andy Lutomirski wrote: >> >> >> > + 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. > > If simple enough and for the sake of accuracy, I could try to issue the > page fault. It seems to me that this entitles calling > force_sig_info_fault in this particular case as opposed to the > force_sig_info(SIGSEGV, SEND_SIG_PRIV, tsk) that do_general_protection > calls. Sure. You could even do it by sending the signal in the emulation code and returning true. --Andy

Dec. 31, 2016

Release plans

by Alexandre Julliard

Folks, Since there was (understandably) very little activity this week, I'm going to skip today's release. I'll release rc4 next Friday; after that, there will probably be a couple more release candidates, since I'm hoping that we can make some more progress on bug fixing once people are back from the holidays. That will put the final 2.0 probably in the second half of January. Happy New Year to everyone! -- Alexandre Julliard julliard(a)winehq.org

Dec. 30, 2016

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

by Ricardo Neri

On Tue, 2016-12-27 at 16:48 -0800, Andy Lutomirski wrote: > 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. Right. Then I will reuse the same addresses. > > > > >> > >> > > >> > 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. In such a case sldt would return a non-null segment selector. I will keep giving the null segment selector in all cases and make a note in the code. > > >> > +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. Agreed. Your statement makes it very clear. I didn't have it that clear in my mind. > > >> > + 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. If we can't read we can't emulate anyways. I will add another note. > > >> > >> > + 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. Right. I have a small query/proposal related to this below. > > >> > + 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. If simple enough and for the sake of accuracy, I could try to issue the page fault. It seems to me that this entitles calling force_sig_info_fault in this particular case as opposed to the force_sig_info(SIGSEGV, SEND_SIG_PRIV, tsk) that do_general_protection calls. Thanks and BR, Ricardo

Dec. 30, 2016

Re: [PATCH] user32: add stub for GetAutoRotationState

by Fabian Maurer

On Friday, December 30, 2016 2:17:34 AM CET Alistair Leslie-Hughes wrote: > Since PAR_STATE is a pointer, %p would be more appropriate. Thanks, fixed it.

Dec. 30, 2016

Re: [PATCH] user32: add stub for GetAutoRotationState

by Alistair Leslie-Hughes

Hi Fabian, On 30/12/16 12:31, Fabian Maurer wrote: > Fixes https://bugs.winehq.org/show_bug.cgi?id=42106 > > Signed-off-by: Fabian Maurer <dark.shadow4(a)web.de> > --- > dlls/user32/misc.c | 14 ++++++++++++++ > dlls/user32/user32.spec | 1 + > include/winuser.h | 13 +++++++++++++ > 3 files changed, 28 insertions(+) > > diff --git a/dlls/user32/misc.c b/dlls/user32/misc.c > index fa0502e25e..15f5c877fa 100644 > --- a/dlls/user32/misc.c > +++ b/dlls/user32/misc.c > @@ -836,6 +836,20 @@ LONG WINAPI GetDisplayConfigBufferSizes(UINT32 flags, UINT32 *num_path_info, UIN > return ERROR_NOT_SUPPORTED; > } > > +/********************************************************************** > + * GetAutoRotationState [USER32.@] > + */ > +BOOL WINAPI GetAutoRotationState(PAR_STATE pState) > +{ > + FIXME("(0x%x): stub\n", pState); Since PAR_STATE is a pointer, %p would be more appropriate. Best Regards Alistair

Dec. 30, 2016

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

by Nikolay Sivov

On 29.12.2016 13:23, Hugh McMaster wrote: > 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. I agree with Ken, I find in-place value renaming useful. However, I see why disabling renaming for keys could be considered as improvement - TreeView is used for navigation most of the time, not for renaming. Accidental renaming definitely happened to me, and that what https://bugs.winehq.org/show_bug.cgi?id=37458 is about. > Thanks for the feedback. > > Hugh >

Dec. 29, 2016

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

by Nikolay Sivov

On 29.12.2016 13:13, Hugh McMaster wrote: > 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. When you use main or context menu LVM_EDITLABEL is sent explicitly, and when you edit using Listview capability it happens on its own, so it should be possible to distinguish those cases. > > [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 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

Re: [v2 2/7] x86/mpx: Fail when implicit zero-displacement is used along with R/EBP

by Ricardo Neri

On Fri, 2016-12-23 at 17:58 -0800, Andy Lutomirski wrote: > On Fri, Dec 23, 2016 at 5:37 PM, Ricardo Neri > <ricardo.neri-calderon(a)linux.intel.com> wrote: > > Section 2.2.1.2 of the Intel 64 and IA-32 Architectures Software > > Developer's Manual volume 2A states that when memory addressing with no > > explicit displacement (i.e, mod part of ModR/M is 0), a SIB byte is used > > and the base of the SIB byte points to (R/EBP) (i.e., base = 5), an > > explicit displacement of 0 must be used. > > > > Make the address decoder to return -EINVAL in such a case. > > > > Cc: Dave Hansen <dave.hansen(a)linux.intel.com> > > Cc: Adam Buchbinder <adam.buchbinder(a)gmail.com> > > Cc: Colin Ian King <colin.king(a)canonical.com> > > Cc: Lorenzo Stoakes <lstoakes(a)gmail.com> > > Cc: Qiaowei Ren <qiaowei.ren(a)intel.com> > > Cc: Ravi V. Shankar <ravi.v.shankar(a)intel.com> > > Cc: x86(a)kernel.org > > Signed-off-by: Ricardo Neri <ricardo.neri-calderon(a)linux.intel.com> > > --- > > arch/x86/mm/mpx.c | 7 +++++++ > > 1 file changed, 7 insertions(+) > > > > diff --git a/arch/x86/mm/mpx.c b/arch/x86/mm/mpx.c > > index 6a75a75..71681d0 100644 > > --- a/arch/x86/mm/mpx.c > > +++ b/arch/x86/mm/mpx.c > > @@ -120,6 +120,13 @@ static int get_reg_offset(struct insn *insn, struct pt_regs *regs, > > > > 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; > > + } > > + > > Now that I've read the cover letter, I see what's going on. This > should not warn -- user code can easily trigger this deliberately. OK, I'll remove it. Are you concerned about the warning printing the calltrace, even only once?

Dec. 27, 2016

Re: [v2 1/7] x86/mpx: Do not use SIB index if index points to R/ESP

by Ricardo Neri

On Fri, 2016-12-23 at 17:57 -0800, Andy Lutomirski wrote: > On Fri, Dec 23, 2016 at 5:37 PM, Ricardo Neri > <ricardo.neri-calderon(a)linux.intel.com> wrote: > > Section 2.2.1.2 of the Intel 64 and IA-32 Architectures Software > > Developer's Manual volume 2A states that when memory addressing is used > > (i.e., mod part of ModR/M is not 3), a SIB byte is used and the index of > > the SIB byte points to the R/ESP (i.e.,index = 4), the index should not be > > used in the computation of the memory address. > > > > An example of such instruction could be > > > > insn -0x80(%rsp) > > > > This is represented as: > > > > [opcode] 4c 24 80 > > > > ModR/M: mod: 1, reg: 1: r/m: 4 (R/ESP) > > SIB 24: sc: 0, index: 100 (R/ESP), base(R/ESP): 100 > > Displacement -0x80 > > > > The correct address is (base) + displacement; no index is used. > > > > Care is taken to allow R12 to be used as index, which is a valid scenario. > > Since I have no idea what this patch has to do with the rest of the > series, I'll ask a question: Thanks for your feedback! I saw in a previous e-mail that you read the cover-letter. :) > > Why isn't this code in the standard x86 instruction decoder? Is the > decoder similarly buggy? I did not find any bug in the instruction decoder. I think the reason this code is not in the decoder is that the decoder only gives you the bytes of the instructions without any meaning. For instance, it gives you the ModRM byte but it does not tell you what register or addressing mode is used. To fully emulate the UMIP instructions I need to give meaning to the ModRM and SIB bytes. Since I was trying many operand combinations, I ran into this issue. Thanks and BR, Ricardo

Dec. 27, 2016

Re: wined3d Add Amd rx 480 binary driver

by Austin English

On Dec 26, 2016 12:04 AM, "Jarkko Korpi" <jarkko_korpi(a)hotmail.com> wrote: http://source.winehq.org/git/wine.git/commit/c63e3261f37e8116e59c70b9bed20a a90c39e8a1 Since that commit Rx 480 supposed to be recognized, but I am using closed source drivers, because the audio patches arent in upstream yet. I found a real problem from SC2 appdatabase and I could confirm it myself. When you start SC2 and go to options it limits your GPU graphics options and complains not enough memory. With the attached patch here the options are not limited anymore. Patches should be sent to wine-patches(a)winehq.org

Dec. 26, 2016

wined3d Add Amd rx 480 binary driver

by Jarkko Korpi

http://source.winehq.org/git/wine.git/commit/c63e3261f37e8116e59c70b9bed20a… Since that commit Rx 480 supposed to be recognized, but I am using closed source drivers, because the audio patches arent in upstream yet. I found a real problem from SC2 appdatabase and I could confirm it myself. When you start SC2 and go to options it limits your GPU graphics options and complains not enough memory. With the attached patch here the options are not limited anymore.

Dec. 26, 2016

(no subject)

by Jarkko Korpi

http://source.winehq.org/git/wine.git/commit/c63e3261f37e8116e59c70b9bed20a… Since that commit Rx 480 supposed to be recognized, but I am using closed source drivers, because the audio patches arent in upstream yet. I found a real problem from SC2 appdatabase and I could confirm it myself. When you start SC2 and go to options it limits your GPU graphics options. With the attached patch here the options are not limited anymore.

Dec. 26, 2016

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

by Masami Hiramatsu

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". Thanks, -- Masami Hiramatsu <mhiramat(a)kernel.org>

Dec. 25, 2016

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

by Masami Hiramatsu

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! :) > --- > 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? Other part looks good to me :) (btw, I saw a kbuild bot warning, would you also test it with CONFIG_X86_DECODER_SELFTEST=y?) 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 > -- Masami Hiramatsu <mhiramat(a)kernel.org>

Dec. 25, 2016

Re: msvcirt: Avoid "array subscript above array bounds" warnings in test_ostrstream and test_strstreambuf.

by Gerald Pfeifer

Hi Piotr, On Fri, 4 Nov 2016, Piotr Caban wrote: > sorry for long reply time. sorry, now it's taken me a while due to travel and other things), and... >> - ok(ssb1.base.ebuf == buffer + 0x7fffffff || ssb1.base.ebuf == (char*)-1, >> + ok((size_t)ssb1.base.ebuf == (size_t)buffer + 0x7fffffff || ssb1.base.ebuf == (char*)-1, > Is casting buffer to char* enough to get away with the warning? I think I > would prefer it to be done this way if it works. ...I believe I tried this and it did not work. On the positive side, Alexandre ran into this now as well and fixed it two days ago, so you and me can close this. ;-) (Thanks, Alexandre!) commit c28816cc378a580258bfb35a8fc8a32d1b40afde Author: Alexandre Julliard <julliard(a)winehq.org> Date: Thu Dec 22 15:07:05 2016 +0100 msvcirt/tests: Silence a compiler warning. Filip, from what I have seen, this should also address what you reported below? Gerald On Thu, 10 Nov 2016, Filip Frackiewicz wrote: > In wine-1.9.22-129-g906e770 I see the array subscript warnings in the > following location: > > ../../../../wine/dlls/msvcirt/tests/msvcirt.c: In function ‘test_strstream’: > ../../../../wine/dlls/msvcirt/tests/msvcirt.c:6861:34: warning: array > subscript is above array bounds [-Warray-bounds] > ok(pssb->base.ebuf == buffer + 0x7fffffff || pssb->base.ebuf == (char*) -1, > ~~~~~~~^~~~~~~~~~~~ > ../../../../wine/dlls/msvcirt/tests/msvcirt.c:6868:35: warning: array > subscript is above array bounds [-Warray-bounds] > ok(pssb->base.epptr == buffer + 0x7fffffff || pssb->base.epptr == > (char*) -1,

Dec. 24, 2016

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

by kbuild test robot

Hi Ricardo, [auto build test ERROR on tip/auto-latest] [also build test ERROR on next-20161223] [cannot apply to tip/x86/core v4.9] [if your patch is applied to the wrong git tree, please drop us a note to help improve the system] url: https://github.com/0day-ci/linux/commits/Ricardo-Neri/x86-enable-User-Mode-… config: x86_64-rhel (attached as .config) compiler: gcc-6 (Debian 6.2.0-3) 6.2.0 20160901 reproduce: # save the attached .config to linux build tree make ARCH=x86_64 All errors (new ones prefixed by >>): In file included from arch/x86/tools/test_get_len.c:27:0: >> arch/x86/include/asm/insn.h:26:23: fatal error: linux/bug.h: No such file or directory #include <linux/bug.h> ^ compilation terminated. -- In file included from tools/include/linux/compiler.h:55:0, from arch/x86/include/asm/insn.h:25, from arch/x86/tools/insn_sanity.c:34: >> tools/include/linux/types.h:28:18: error: conflicting types for 'u64' typedef uint64_t u64; ^~~ In file included from /usr/include/asm-generic/types.h:6:0, from /usr/include/x86_64-linux-gnu/asm/types.h:4, from tools/include/linux/types.h:9, from tools/include/linux/compiler.h:55, from arch/x86/include/asm/insn.h:25, from arch/x86/tools/insn_sanity.c:34: include/asm-generic/int-ll64.h:25:28: note: previous declaration of 'u64' was here typedef unsigned long long u64; ^~~ In file included from tools/include/linux/compiler.h:55:0, from arch/x86/include/asm/insn.h:25, from arch/x86/tools/insn_sanity.c:34: >> tools/include/linux/types.h:29:17: error: conflicting types for 's64' typedef int64_t s64; ^~~ In file included from /usr/include/asm-generic/types.h:6:0, from /usr/include/x86_64-linux-gnu/asm/types.h:4, from tools/include/linux/types.h:9, from tools/include/linux/compiler.h:55, from arch/x86/include/asm/insn.h:25, from arch/x86/tools/insn_sanity.c:34: include/asm-generic/int-ll64.h:24:26: note: previous declaration of 's64' was here typedef signed long long s64; ^~~ In file included from tools/include/linux/types.h:4:0, from tools/include/linux/compiler.h:55, from arch/x86/include/asm/insn.h:25, from arch/x86/tools/insn_sanity.c:34: >> include/linux/stddef.h:10:2: error: expected identifier before numeric constant false = 0, ^ In file included from arch/x86/include/asm/ptrace.h:5:0, from arch/x86/include/asm/insn.h:28, from arch/x86/tools/insn_sanity.c:34: >> arch/x86/include/asm/page_types.h:61:15: error: unknown type name 'phys_addr_t' static inline phys_addr_t get_max_mapped(void) ^~~~~~~~~~~ arch/x86/include/asm/page_types.h: In function 'get_max_mapped': >> arch/x86/include/asm/page_types.h:63:10: error: 'phys_addr_t' undeclared (first use in this function) return (phys_addr_t)max_pfn_mapped << PAGE_SHIFT; ^~~~~~~~~~~ arch/x86/include/asm/page_types.h:63:10: note: each undeclared identifier is reported only once for each function it appears in >> arch/x86/include/asm/page_types.h:63:22: error: expected ';' before 'max_pfn_mapped' return (phys_addr_t)max_pfn_mapped << PAGE_SHIFT; ^~~~~~~~~~~~~~ In file included from arch/x86/include/asm/insn.h:28:0, from arch/x86/tools/insn_sanity.c:34: arch/x86/include/asm/ptrace.h: At top level: >> arch/x86/include/asm/ptrace.h:33:8: error: redefinition of 'struct pt_regs' struct pt_regs { ^~~~~~~ In file included from arch/x86/include/asm/insn.h:26:0, from arch/x86/tools/insn_sanity.c:34: include/linux/bug.h:13:8: note: originally defined here struct pt_regs; ^~~~~~~ vim +33 arch/x86/include/asm/ptrace.h 92bc2056 include/asm-x86/ptrace.h Harvey Harrison 2008-02-08 27 unsigned long sp; 92bc2056 include/asm-x86/ptrace.h Harvey Harrison 2008-02-08 28 unsigned long ss; 65ea5b03 include/asm-x86/ptrace.h H. Peter Anvin 2008-01-30 29 }; 8fc37f2c include/asm-x86/ptrace.h Thomas Gleixner 2007-10-23 30 8fc37f2c include/asm-x86/ptrace.h Thomas Gleixner 2007-10-23 31 #else /* __i386__ */ 8fc37f2c include/asm-x86/ptrace.h Thomas Gleixner 2007-10-23 32 65ea5b03 include/asm-x86/ptrace.h H. Peter Anvin 2008-01-30 @33 struct pt_regs { e90e147c arch/x86/include/asm/ptrace.h Denys Vlasenko 2015-02-26 34 /* e90e147c arch/x86/include/asm/ptrace.h Denys Vlasenko 2015-02-26 35 * C ABI says these regs are callee-preserved. They aren't saved on kernel entry e90e147c arch/x86/include/asm/ptrace.h Denys Vlasenko 2015-02-26 36 * unless syscall needs a complete, fully filled "struct pt_regs". :::::: The code at line 33 was first introduced by commit :::::: 65ea5b0349903585bfed9720fa06f5edb4f1cd25 x86: rename the struct pt_regs members for 32/64-bit consistency :::::: TO: H. Peter Anvin <hpa(a)zytor.com> :::::: CC: Ingo Molnar <mingo(a)elte.hu> --- 0-DAY kernel test infrastructure Open Source Technology Center https://lists.01.org/pipermail/kbuild-all Intel Corporation

Dec. 24, 2016

Re: [v2 7/7] x86: Enable User-Mode Instruction Prevention

by kbuild test robot

Hi Ricardo, [auto build test WARNING on tip/auto-latest] [also build test WARNING on next-20161223] [cannot apply to tip/x86/core v4.9] [if your patch is applied to the wrong git tree, please drop us a note to help improve the system] url: https://github.com/0day-ci/linux/commits/Ricardo-Neri/x86-enable-User-Mode-… config: x86_64-acpi-redef (attached as .config) compiler: gcc-6 (Debian 6.2.0-3) 6.2.0 20160901 reproduce: # save the attached .config to linux build tree make ARCH=x86_64 All warnings (new ones prefixed by >>): arch/x86/kernel/umip.c: In function '__identify_insn': >> arch/x86/kernel/umip.c:65:1: warning: control reaches end of non-void function [-Wreturn-type] } ^ vim +65 arch/x86/kernel/umip.c 78301541 Ricardo Neri 2016-12-23 49 return UMIP_SGDT; 78301541 Ricardo Neri 2016-12-23 50 case 1: 78301541 Ricardo Neri 2016-12-23 51 return UMIP_SIDT; 78301541 Ricardo Neri 2016-12-23 52 case 4: 78301541 Ricardo Neri 2016-12-23 53 return UMIP_SMSW; 78301541 Ricardo Neri 2016-12-23 54 default: 78301541 Ricardo Neri 2016-12-23 55 return -EINVAL; 78301541 Ricardo Neri 2016-12-23 56 } 78301541 Ricardo Neri 2016-12-23 57 } else if (insn->opcode.bytes[1] == 0x0) { 78301541 Ricardo Neri 2016-12-23 58 if (X86_MODRM_REG(insn->modrm.value) == 0) 78301541 Ricardo Neri 2016-12-23 59 return UMIP_SLDT; 78301541 Ricardo Neri 2016-12-23 60 else if (X86_MODRM_REG(insn->modrm.value) == 1) 78301541 Ricardo Neri 2016-12-23 61 return UMIP_STR; 78301541 Ricardo Neri 2016-12-23 62 else 78301541 Ricardo Neri 2016-12-23 63 return -EINVAL; 78301541 Ricardo Neri 2016-12-23 64 } 78301541 Ricardo Neri 2016-12-23 @65 } 78301541 Ricardo Neri 2016-12-23 66 78301541 Ricardo Neri 2016-12-23 67 static int __emulate_umip_insn(struct insn *insn, enum umip_insn umip_inst, 78301541 Ricardo Neri 2016-12-23 68 unsigned char *data, int *data_size) 78301541 Ricardo Neri 2016-12-23 69 { 78301541 Ricardo Neri 2016-12-23 70 unsigned long const *dummy_base_addr; 78301541 Ricardo Neri 2016-12-23 71 unsigned short dummy_limit = 0; 78301541 Ricardo Neri 2016-12-23 72 unsigned short dummy_value = 0; 78301541 Ricardo Neri 2016-12-23 73 :::::: The code at line 65 was first introduced by commit :::::: 7830154191c35fde49ef59c2b9328f6b32203be4 x86: Add emulation code for UMIP instructions :::::: TO: Ricardo Neri <ricardo.neri-calderon(a)linux.intel.com> :::::: CC: 0day robot <fengguang.wu(a)intel.com> --- 0-DAY kernel test infrastructure Open Source Technology Center https://lists.01.org/pipermail/kbuild-all Intel Corporation

Dec. 24, 2016

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

by kbuild test robot

Hi Ricardo, [auto build test WARNING on tip/auto-latest] [also build test WARNING on v4.9 next-20161223] [cannot apply to tip/x86/core] [if your patch is applied to the wrong git tree, please drop us a note to help improve the system] url: https://github.com/0day-ci/linux/commits/Ricardo-Neri/x86-enable-User-Mode-… config: x86_64-randconfig-x008-201651 (attached as .config) compiler: gcc-6 (Debian 6.2.0-3) 6.2.0 20160901 reproduce: # save the attached .config to linux build tree make ARCH=x86_64 All warnings (new ones prefixed by >>): >> warning: objtool: x86 instruction decoder differs from kernel --- 0-DAY kernel test infrastructure Open Source Technology Center https://lists.01.org/pipermail/kbuild-all Intel Corporation

Dec. 24, 2016

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

by Andy Lutomirski

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. > > 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.

Dec. 24, 2016

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

by Andy Lutomirski

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. 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.

Dec. 24, 2016

Re: [v2 2/7] x86/mpx: Fail when implicit zero-displacement is used along with R/EBP

by Andy Lutomirski

On Fri, Dec 23, 2016 at 5:37 PM, Ricardo Neri <ricardo.neri-calderon(a)linux.intel.com> wrote: > Section 2.2.1.2 of the Intel 64 and IA-32 Architectures Software > Developer's Manual volume 2A states that when memory addressing with no > explicit displacement (i.e, mod part of ModR/M is 0), a SIB byte is used > and the base of the SIB byte points to (R/EBP) (i.e., base = 5), an > explicit displacement of 0 must be used. > > Make the address decoder to return -EINVAL in such a case. > > Cc: Dave Hansen <dave.hansen(a)linux.intel.com> > Cc: Adam Buchbinder <adam.buchbinder(a)gmail.com> > Cc: Colin Ian King <colin.king(a)canonical.com> > Cc: Lorenzo Stoakes <lstoakes(a)gmail.com> > Cc: Qiaowei Ren <qiaowei.ren(a)intel.com> > Cc: Ravi V. Shankar <ravi.v.shankar(a)intel.com> > Cc: x86(a)kernel.org > Signed-off-by: Ricardo Neri <ricardo.neri-calderon(a)linux.intel.com> > --- > arch/x86/mm/mpx.c | 7 +++++++ > 1 file changed, 7 insertions(+) > > diff --git a/arch/x86/mm/mpx.c b/arch/x86/mm/mpx.c > index 6a75a75..71681d0 100644 > --- a/arch/x86/mm/mpx.c > +++ b/arch/x86/mm/mpx.c > @@ -120,6 +120,13 @@ static int get_reg_offset(struct insn *insn, struct pt_regs *regs, > > 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; > + } > + Now that I've read the cover letter, I see what's going on. This should not warn -- user code can easily trigger this deliberately.

Dec. 24, 2016

Re: [v2 1/7] x86/mpx: Do not use SIB index if index points to R/ESP

by Andy Lutomirski

On Fri, Dec 23, 2016 at 5:37 PM, Ricardo Neri <ricardo.neri-calderon(a)linux.intel.com> wrote: > Section 2.2.1.2 of the Intel 64 and IA-32 Architectures Software > Developer's Manual volume 2A states that when memory addressing is used > (i.e., mod part of ModR/M is not 3), a SIB byte is used and the index of > the SIB byte points to the R/ESP (i.e.,index = 4), the index should not be > used in the computation of the memory address. > > An example of such instruction could be > > insn -0x80(%rsp) > > This is represented as: > > [opcode] 4c 24 80 > > ModR/M: mod: 1, reg: 1: r/m: 4 (R/ESP) > SIB 24: sc: 0, index: 100 (R/ESP), base(R/ESP): 100 > Displacement -0x80 > > The correct address is (base) + displacement; no index is used. > > Care is taken to allow R12 to be used as index, which is a valid scenario. Since I have no idea what this patch has to do with the rest of the series, I'll ask a question: Why isn't this code in the standard x86 instruction decoder? Is the decoder similarly buggy?

Dec. 24, 2016

[v2 7/7] x86: Enable User-Mode Instruction Prevention

by Ricardo Neri

User_mode Instruction Prevention (UMIP) is enabled by setting/clearing a bit in %cr4. It makes sense to enable UMIP at some point while booting, before user spaces come up. Like SMAP and SMEP, is not critical to have it enabled very early during boot. This is because UMIP is relevant only when there is a userspace to be protected from. Given the similarities in relevance, it makes sense to enable UMIP along with SMAP and SMEP. UMIP is enabled by default. It can be disabled by adding clearcpuid=514 to the kernel parameters. 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/Kconfig | 10 ++++++++++ arch/x86/kernel/cpu/common.c | 16 +++++++++++++++- 2 files changed, 25 insertions(+), 1 deletion(-) diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig index e487493..bae1a8f 100644 --- a/arch/x86/Kconfig +++ b/arch/x86/Kconfig @@ -1733,6 +1733,16 @@ config X86_SMAP If unsure, say Y. +config X86_INTEL_UMIP + def_bool y + depends on CPU_SUP_INTEL + prompt "User Mode Instruction Prevention" if EXPERT + ---help--- + The User Mode Instruction Prevention (UMIP) is a security + feature in newer Intel processors. If enabled, a general + protection fault is issued if the instructions SGDT, SLDT, + SIDT, SMSW and STR are executed in user mode. + config X86_INTEL_MPX prompt "Intel MPX (Memory Protection Extensions)" def_bool n diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c index dc1697c..b38a639 100644 --- a/arch/x86/kernel/cpu/common.c +++ b/arch/x86/kernel/cpu/common.c @@ -306,6 +306,19 @@ static __always_inline void setup_smap(struct cpuinfo_x86 *c) } } +static __always_inline void setup_umip(struct cpuinfo_x86 *c) +{ + if (cpu_feature_enabled(X86_FEATURE_UMIP) && + cpu_has(c, X86_FEATURE_UMIP)) + cr4_set_bits(X86_CR4_UMIP); + else + /* + * Make sure UMIP is disabled in case it was enabled in a + * previous boot (e.g., via kexec). + */ + cr4_clear_bits(X86_CR4_UMIP); +} + /* * Protection Keys are not available in 32-bit mode. */ @@ -1059,9 +1072,10 @@ static void identify_cpu(struct cpuinfo_x86 *c) /* Disable the PN if appropriate */ squash_the_stupid_serial_number(c); - /* Set up SMEP/SMAP */ + /* Set up SMEP/SMAP/UMIP */ setup_smep(c); setup_smap(c); + setup_umip(c); /* * The vendor-specific functions might have changed features. -- 2.9.3

Dec. 24, 2016

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

by Ricardo Neri

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; + if (v8086_mode(regs)) { local_irq_enable(); handle_vm86_fault((struct kernel_vm86_regs *) regs, error_code); -- 2.9.3

Dec. 24, 2016

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

by Ricardo Neri

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. 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. The instruction smsw is emulated to return zero. 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/include/asm/umip.h | 16 +++++ arch/x86/kernel/Makefile | 1 + arch/x86/kernel/umip.c | 170 ++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 187 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 0000000..7bcaca6 --- /dev/null +++ b/arch/x86/include/asm/umip.h @@ -0,0 +1,16 @@ +#ifndef _ASM_X86_UMIP_H +#define _ASM_X86_UMIP_H + +#include <linux/types.h> +#include <asm/ptrace.h> +#include <asm/insn.h> + +#ifdef CONFIG_X86_INTEL_UMIP +int fixup_umip_exception(struct pt_regs *regs); +#else +static inline int fixup_umip_exception(struct pt_regs *regs) +{ + return -EINVAL; +} +#endif /* CONFIG_X86_INTEL_UMIP */ +#endif /* _ASM_X86_UMIP_H */ diff --git a/arch/x86/kernel/Makefile b/arch/x86/kernel/Makefile index 581386c..c4aec02 100644 --- a/arch/x86/kernel/Makefile +++ b/arch/x86/kernel/Makefile @@ -124,6 +124,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 obj-y += unwind_frame.o diff --git a/arch/x86/kernel/umip.c b/arch/x86/kernel/umip.c new file mode 100644 index 0000000..a104aea --- /dev/null +++ b/arch/x86/kernel/umip.c @@ -0,0 +1,170 @@ +/* + * umip.c Emulation for instruction protected by the Intel User-Mode + * Instruction Prevention. The instructions are: + * sgdt + * sldt + * sidt + * str + * smsw + * + * Copyright (c) 2016, Intel Corporation. + * Ricardo Neri <ricardo.neri(a)linux.intel.com> + */ + +#include <linux/compiler.h> +#include <linux/bug.h> +#include <linux/uaccess.h> +#include <linux/err.h> +#include <asm/ptrace.h> +#include <asm/umip.h> +#include <linux/thread_info.h> +#include <linux/thread_info.h> + +/* + * The address of this dummy values need to be readable by + * the user space + */ + +static const long umip_dummy_gdt_base; +static const long umip_dummy_idt_base; + +enum umip_insn { + UMIP_SGDT = 0, /* opcode 0f 01 ModR/M reg 0 */ + UMIP_SIDT, /* opcode 0f 01 ModR/M reg 1 */ + UMIP_SLDT, /* opcode 0f 00 ModR/M reg 0 */ + UMIP_SMSW, /* opcode 0f 01 ModR/M reg 4 */ + UMIP_STR, /* opcode 0f 00 ModR/M reg 1 */ +}; + +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; + } +} + +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"); + return -EINVAL; + } + /* 16-bit operand. fill most significant byte with zeros */ + if (insn->opnd_bytes == 2) + dummy_base_addr = (unsigned long *) + ((unsigned long) + dummy_base_addr & 0xffffff); + memcpy(data + 2, &dummy_base_addr, sizeof(dummy_base_addr)); + memcpy(data, &dummy_limit, sizeof(dummy_limit)); + *data_size = sizeof(dummy_base_addr) + sizeof(dummy_limit); + break; + /* + * These three instructions return a 16-bit value. We return + * all zeros. This is equivalent to a null descriptor for + * str and sldt. For smsw, is equivalent to an all-zero CR0. + */ + case UMIP_SLDT: + case UMIP_SMSW: + case UMIP_STR: + /* if operand is a register, it is zero-extended*/ + if (X86_MODRM_MOD(insn->modrm.value) == 3) { + memset(data, 0, insn->opnd_bytes); + *data_size = insn->opnd_bytes; + } else + *data_size = sizeof(dummy_value); + memcpy(data, &dummy_value, sizeof(dummy_value)); + break; + default: + return -EINVAL; + } + return 0; +} + +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); + 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)); + 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; + insn_init(&insn, buf, nr_copied, x86_64); + insn_get_length(&insn); + if (nr_copied < insn.length) + return -EFAULT; + + 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; + } + + /* increase IP to let the program keep going */ + regs->ip += insn.length; + return 0; +} -- 2.9.3

Dec. 24, 2016

[v2 4/7] x86/cpufeature: Add User-Mode Instruction Prevention definitions

by Ricardo Neri

User-Mode Instruction Prevention is a security feature present in new Intel processors that, when set, prevents the execution of a subset of instructions if such instructions are executed in user mode (CPL > 0). Attempting to execute such instructions causes a general protection exception. The subset of instructions comprises: * SGDT - Store Global Descriptor Table * SIDT - Store Interrupt Descriptor Table * SLDT - Store Local Descriptor Table * SMSW - Store Machine Status Word * STR - Store Task Register This feature is also added to the list of disabled-features to allow a cleaner handling of build-time configuration. 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/include/asm/cpufeatures.h | 1 + arch/x86/include/asm/disabled-features.h | 8 +++++++- arch/x86/include/uapi/asm/processor-flags.h | 2 ++ 3 files changed, 10 insertions(+), 1 deletion(-) diff --git a/arch/x86/include/asm/cpufeatures.h b/arch/x86/include/asm/cpufeatures.h index eafee31..be61bf9 100644 --- a/arch/x86/include/asm/cpufeatures.h +++ b/arch/x86/include/asm/cpufeatures.h @@ -286,6 +286,7 @@ /* Intel-defined CPU features, CPUID level 0x00000007:0 (ecx), word 16 */ #define X86_FEATURE_AVX512VBMI (16*32+ 1) /* AVX512 Vector Bit Manipulation instructions*/ +#define X86_FEATURE_UMIP (16*32+ 2) /* User Mode Instruction Protection */ #define X86_FEATURE_PKU (16*32+ 3) /* Protection Keys for Userspace */ #define X86_FEATURE_OSPKE (16*32+ 4) /* OS Protection Keys Enable */ #define X86_FEATURE_RDPID (16*32+ 22) /* RDPID instruction */ diff --git a/arch/x86/include/asm/disabled-features.h b/arch/x86/include/asm/disabled-features.h index 85599ad..4707445 100644 --- a/arch/x86/include/asm/disabled-features.h +++ b/arch/x86/include/asm/disabled-features.h @@ -16,6 +16,12 @@ # define DISABLE_MPX (1<<(X86_FEATURE_MPX & 31)) #endif +#ifdef CONFIG_X86_INTEL_UMIP +# define DISABLE_UMIP 0 +#else +# define DISABLE_UMIP (1<<(X86_FEATURE_UMIP & 31)) +#endif + #ifdef CONFIG_X86_64 # define DISABLE_VME (1<<(X86_FEATURE_VME & 31)) # define DISABLE_K6_MTRR (1<<(X86_FEATURE_K6_MTRR & 31)) @@ -55,7 +61,7 @@ #define DISABLED_MASK13 0 #define DISABLED_MASK14 0 #define DISABLED_MASK15 0 -#define DISABLED_MASK16 (DISABLE_PKU|DISABLE_OSPKE) +#define DISABLED_MASK16 (DISABLE_PKU|DISABLE_OSPKE|DISABLE_UMIP) #define DISABLED_MASK17 0 #define DISABLED_MASK_CHECK BUILD_BUG_ON_ZERO(NCAPINTS != 18) diff --git a/arch/x86/include/uapi/asm/processor-flags.h b/arch/x86/include/uapi/asm/processor-flags.h index 567de50..d2c2af8 100644 --- a/arch/x86/include/uapi/asm/processor-flags.h +++ b/arch/x86/include/uapi/asm/processor-flags.h @@ -104,6 +104,8 @@ #define X86_CR4_OSFXSR _BITUL(X86_CR4_OSFXSR_BIT) #define X86_CR4_OSXMMEXCPT_BIT 10 /* enable unmasked SSE exceptions */ #define X86_CR4_OSXMMEXCPT _BITUL(X86_CR4_OSXMMEXCPT_BIT) +#define X86_CR4_UMIP_BIT 11 /* enable UMIP support */ +#define X86_CR4_UMIP _BITUL(X86_CR4_UMIP_BIT) #define X86_CR4_VMXE_BIT 13 /* enable VMX virtualization */ #define X86_CR4_VMXE _BITUL(X86_CR4_VMXE_BIT) #define X86_CR4_SMXE_BIT 14 /* enable safer mode (TXT) */ -- 2.9.3

Dec. 24, 2016

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

by Ricardo Neri

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. Cc: Dave Hansen <dave.hansen(a)linux.intel.com> Cc: Adam Buchbinder <adam.buchbinder(a)gmail.com> Cc: Colin Ian King <colin.king(a)canonical.com> Cc: Lorenzo Stoakes <lstoakes(a)gmail.com> Cc: Qiaowei Ren <qiaowei.ren(a)intel.com> Cc: Arnaldo Carvalho de Melo <acme(a)redhat.com> Cc: Masami Hiramatsu <mhiramat(a)kernel.org> Cc: Adrian Hunter <adrian.hunter(a)intel.com> Cc: Kees Cook <keescook(a)chromium.org> Cc: Thomas Garnier <thgarnie(a)google.com> Cc: Peter Zijlstra <peterz(a)infradead.org> Cc: Borislav Petkov <bp(a)suse.de> Cc: Dmitry Vyukov <dvyukov(a)google.com> Cc: Ravi V. Shankar <ravi.v.shankar(a)intel.com> Cc: x86(a)kernel.org Signed-off-by: Ricardo Neri <ricardo.neri-calderon(a)linux.intel.com> --- 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); /* 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. 24, 2016

[v2 2/7] x86/mpx: Fail when implicit zero-displacement is used along with R/EBP

by Ricardo Neri

Section 2.2.1.2 of the Intel 64 and IA-32 Architectures Software Developer's Manual volume 2A states that when memory addressing with no explicit displacement (i.e, mod part of ModR/M is 0), a SIB byte is used and the base of the SIB byte points to (R/EBP) (i.e., base = 5), an explicit displacement of 0 must be used. Make the address decoder to return -EINVAL in such a case. Cc: Dave Hansen <dave.hansen(a)linux.intel.com> Cc: Adam Buchbinder <adam.buchbinder(a)gmail.com> Cc: Colin Ian King <colin.king(a)canonical.com> Cc: Lorenzo Stoakes <lstoakes(a)gmail.com> Cc: Qiaowei Ren <qiaowei.ren(a)intel.com> Cc: Ravi V. Shankar <ravi.v.shankar(a)intel.com> Cc: x86(a)kernel.org Signed-off-by: Ricardo Neri <ricardo.neri-calderon(a)linux.intel.com> --- arch/x86/mm/mpx.c | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/arch/x86/mm/mpx.c b/arch/x86/mm/mpx.c index 6a75a75..71681d0 100644 --- a/arch/x86/mm/mpx.c +++ b/arch/x86/mm/mpx.c @@ -120,6 +120,13 @@ static int get_reg_offset(struct insn *insn, struct pt_regs *regs, 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; -- 2.9.3

Dec. 24, 2016

[v2 1/7] x86/mpx: Do not use SIB index if index points to R/ESP

by Ricardo Neri

Section 2.2.1.2 of the Intel 64 and IA-32 Architectures Software Developer's Manual volume 2A states that when memory addressing is used (i.e., mod part of ModR/M is not 3), a SIB byte is used and the index of the SIB byte points to the R/ESP (i.e.,index = 4), the index should not be used in the computation of the memory address. An example of such instruction could be insn -0x80(%rsp) This is represented as: [opcode] 4c 24 80 ModR/M: mod: 1, reg: 1: r/m: 4 (R/ESP) SIB 24: sc: 0, index: 100 (R/ESP), base(R/ESP): 100 Displacement -0x80 The correct address is (base) + displacement; no index is used. Care is taken to allow R12 to be used as index, which is a valid scenario. Cc: Dave Hansen <dave.hansen(a)linux.intel.com> Cc: Adam Buchbinder <adam.buchbinder(a)gmail.com> Cc: Colin Ian King <colin.king(a)canonical.com> Cc: Lorenzo Stoakes <lstoakes(a)gmail.com> Cc: Qiaowei Ren <qiaowei.ren(a)intel.com> Cc: Ravi V. Shankar <ravi.v.shankar(a)intel.com> Cc: x86(a)kernel.org Signed-off-by: Ricardo Neri <ricardo.neri-calderon(a)linux.intel.com> --- arch/x86/mm/mpx.c | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/arch/x86/mm/mpx.c b/arch/x86/mm/mpx.c index 324e571..6a75a75 100644 --- a/arch/x86/mm/mpx.c +++ b/arch/x86/mm/mpx.c @@ -109,6 +109,13 @@ static int get_reg_offset(struct insn *insn, struct pt_regs *regs, 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: @@ -161,7 +168,10 @@ static void __user *mpx_get_addr_ref(struct insn *insn, struct pt_regs *regs) goto out_err; base = regs_get_register(regs, base_offset); - indx = regs_get_register(regs, indx_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); -- 2.9.3

Dec. 24, 2016

[v2 0/7] x86: enable User-Mode Instruction Prevention

by Ricardo Neri

This is v2 of my first submission done a while ago[1]. I apologize for the delay. User-Mode Instruction Prevention (UMIP) is a security feature present in new Intel Processors. If enabled, it prevents the execution of certain instructions if the Current Privilege Level (CPL) is greater than 0. If these instructions were executed while in CPL > 0, user space applications could have access to system-wide settings such as the global and local descriptor tables, the segment selectors to the current task state and the local descriptor table. These are the instructions covered by UMIP: * SGDT - Store Global Descriptor Table * SIDT - Store Interrupt Descriptor Table * SLDT - Store Local Descriptor Table * SMSW - Store Machine Status Word * STR - Store Task Register If any of these instructions is executed with CPL > 0, a general protection exception is issued when UMIP is enabled. There is a caveat, however. Certain applications rely on some of these instructions to function. An example of this are applications that use WineHQ[2]. For instance, these applications rely on sidt returning a non- accesible memory location[3]. During the discussions, it was proposed that the fault could be relied to the user-space and perform the emulation in user-mode. However, this would break existing applications until, for instance, they update to a new WineHQ version. However, this approach would require UMIP to be disabled by default. The concensus in this forum is to always enable it. This patchset initially treated tasks running in virtual-8086 mode as a special case. However, I received clarification that DOSEMU[4] does not support applications that use these instructions. It relies on WineHQ for this[4]. Furthermore, the applications for which the concern was raised run in protected mode [3]. This version keeps UMIP enabled at all times and by default. If a general protection fault caused by the instructions protected by UMIP is detected, such fault will be fixed-up by returning dummy values as follows: * SGDT and SIDT return a base address to a dummy location in kernel memory and a limit of 0. * STR, SLDT returns 0 as the segment selector. This seems OK since we are providing a dummy value as the base address of the global descriptor table. * SMSW returns 0. Lastly, I found very useful the code for Intel MPX (Memory Protection Extensions) used to parse opcodes and the memory locations contained in the general purpose registers when used as operands. I put some of this code in a separate file that both MPX and UMIP can access and avoid code duplication. While here, I fixed two small bugs that I found in the MPX implementation. The code that I used to test the emulated instructions can be found in [6]. [1]. https://lwn.net/Articles/705877/ [2]. https://www.winehq.org/ [3]. https://www.winehq.org/pipermail/wine-devel/2016-November/115320.html [4]. http://www.dosemu.org/ [5]. http://marc.info/?l=linux-kernel&m=147876798717927&w=2 [6]. https://github.com/01org/luv-yocto/tree/rneri/umip/meta-luv/recipes-core/um… Thanks and BR, Ricardo Cc: Andy Lutomirski <luto(a)kernel.org> Cc: Andrew Morton <akpm(a)linux-foundation.org> 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: Vlastimil Babka <vbabka(a)suse.cz> Cc: Shuah Khan <shuah(a)kernel.org> Cc: Paolo Bonzini <pbonzini(a)redhat.com> Cc: Liang Z Li <liang.z.li(a)intel.com> Cc: x86(a)kernel.org Cc: linux-msdos(a)vger.kernel.org Changes since V1: * Virtual-8086 mode tasks are not treated in a special manner. All code for this purpose was removed. * Instead of attempting to disable UMIP during a context switch or when entering virtual-8086 mode, UMIP remains enabled all the time. General protection faults that occur are fixed-up by returning dummy values as detailed above. * Removed umip= kernel parameter in favor of using clearcpuid=514 to disable UMIP. * Removed selftests designed to detect the absence of SIGSEGV signals when running in virtual-8086 mode. * Reused code from MPX to decode instructions operands. For this purpose code was put in a common location. * Fixed two bugs in MPX code that decodes operands. Ricardo Neri (7): x86/mpx: Do not use SIB index if index points to R/ESP x86/mpx: Fail when implicit zero-displacement is used along with R/EBP x86/mpx, x86/insn: Relocate insn util functions to a new insn-utils x86/cpufeature: Add User-Mode Instruction Prevention definitions x86: Add emulation code for UMIP instructions x86/traps: Fixup general protection faults caused by UMIP x86: Enable User-Mode Instruction Prevention arch/x86/Kconfig | 10 ++ arch/x86/include/asm/cpufeatures.h | 1 + arch/x86/include/asm/disabled-features.h | 8 +- arch/x86/include/asm/insn.h | 6 + arch/x86/include/asm/umip.h | 16 +++ arch/x86/include/uapi/asm/processor-flags.h | 2 + arch/x86/kernel/Makefile | 1 + arch/x86/kernel/cpu/common.c | 16 ++- arch/x86/kernel/traps.c | 4 + arch/x86/kernel/umip.c | 170 ++++++++++++++++++++++++++++ arch/x86/lib/Makefile | 2 +- arch/x86/lib/insn-utils.c | 148 ++++++++++++++++++++++++ arch/x86/mm/mpx.c | 119 +------------------ 13 files changed, 382 insertions(+), 121 deletions(-) create mode 100644 arch/x86/include/asm/umip.h create mode 100644 arch/x86/kernel/umip.c create mode 100644 arch/x86/lib/insn-utils.c -- 2.9.3

Dec. 24, 2016

Re: [PATCH] mscoree: Free argument buffer on error (Coverity)

by Vincent Povirk

I'm out visiting family and don't have access to a machine that can build or run Wine. I'll be able to do reviews again next year. FWIW, by purely visual inspection this looks fine to me.

Dec. 22, 2016

Re: [PATCH] msvcrt: Pass WCHAR strings to MessageBoxIndirectW.

by Nikolay Sivov

On Thu, 22 Dec 2016 14:38:19 -0700 Alex Henrie <alexhenrie24(a)gmail.com> wrote: > Cc: Piotr Caban <piotr(a)codeweavers.com> > Cc: Nikolay Sivov <bunglehead(a)gmail.com> > > Fixes https://bugs.winehq.org/show_bug.cgi?id=42057 > > I figured this would be a good time to get rid of the misleading > Hungarian notation and move the string into the single function that > uses it. > > v2: Switch to WCHAR type. > > Signed-off-by: Alex Henrie <alexhenrie24(a)gmail.com> > --- > dlls/msvcrt/exit.c | 10 +++++----- > 1 file changed, 5 insertions(+), 5 deletions(-) > > diff --git a/dlls/msvcrt/exit.c b/dlls/msvcrt/exit.c > index 9ba0891806..ad5a78d54d 100644 > --- a/dlls/msvcrt/exit.c > +++ b/dlls/msvcrt/exit.c > @@ -41,8 +41,6 @@ typedef struct MSVCRT__onexit_table_t > MSVCRT__onexit_t *_end; > } MSVCRT__onexit_table_t; > > -static const char szMsgBoxTitle[] = "Wine C++ Runtime Library"; > - > extern int MSVCRT_app_type; > extern MSVCRT_wchar_t *MSVCRT__wpgmptr; > > @@ -112,10 +110,12 @@ void CDECL MSVCRT__exit(int exitcode) > /* Print out an error message with an option to debug */ > static void DoMessageBoxW(const MSVCRT_wchar_t *lead, const MSVCRT_wchar_t *message) > { > - static const MSVCRT_wchar_t message_format[] = {'%','s','\n','\n','P','r','o','g','r','a','m',':',' ','%','s','\n', > + static const WCHAR message_format[] = {'%','s','\n','\n','P','r','o','g','r','a','m',':',' ','%','s','\n', > '%','s','\n','\n','P','r','e','s','s',' ','O','K',' ','t','o',' ','e','x','i','t',' ','t','h','e',' ', > 'p','r','o','g','r','a','m',',',' ','o','r',' ','C','a','n','c','e','l',' ','t','o',' ','s','t','a','r','t',' ', > 't','h','e',' ','W','i','n','e',' ','d','e','b','b','u','g','e','r','.','\n',0}; > + static const WCHAR title[] = > + {'W','i','n','e',' ','C','+','+',' ','R','u','n','t','i','m','e',' ','L','i','b','r','a','r','y',0}; 'message_format' is used with MSVCRT__snwprintf, so it should be wchar_t strictly speaking. > > MSGBOXPARAMSW msgbox; > MSVCRT_wchar_t text[2048]; Another existing bug is wrong buffer length here: --- MSVCRT__snwprintf(text,sizeof(text),message_format, lead, MSVCRT__wpgmptr, message); --- Could be a separate patch I guess. > @@ -126,8 +126,8 @@ static void DoMessageBoxW(const MSVCRT_wchar_t *lead, const MSVCRT_wchar_t *mess > msgbox.cbSize = sizeof(msgbox); > msgbox.hwndOwner = GetActiveWindow(); > msgbox.hInstance = 0; > - msgbox.lpszText = (LPCWSTR)text; > - msgbox.lpszCaption = (LPCWSTR)szMsgBoxTitle; > + msgbox.lpszText = text; > + msgbox.lpszCaption = title; What I meant is that you can remove 'text' cast without changing its type. > msgbox.dwStyle = MB_OKCANCEL|MB_ICONERROR; > msgbox.lpszIcon = NULL; > msgbox.dwContextHelpId = 0;

Dec. 22, 2016

Re: [PATCH] msvcrt: Pass wide char caption string to MessageBoxIndirectW.

by Nikolay Sivov

On Thu, 22 Dec 2016 14:20:26 -0700 Alex Henrie <alexhenrie24(a)gmail.com> wrote: > Cc: Piotr Caban <piotr(a)codeweavers.com> > > Fixes https://bugs.winehq.org/show_bug.cgi?id=42057 > > I figured this would be a good time to get rid of the misleading > Hungarian notation and move the string into the single function that > uses it. > > Signed-off-by: Alex Henrie <alexhenrie24(a)gmail.com> > --- > dlls/msvcrt/exit.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/dlls/msvcrt/exit.c b/dlls/msvcrt/exit.c > index 9ba0891806..3bfcd15967 100644 > --- a/dlls/msvcrt/exit.c > +++ b/dlls/msvcrt/exit.c > @@ -41,8 +41,6 @@ typedef struct MSVCRT__onexit_table_t > MSVCRT__onexit_t *_end; > } MSVCRT__onexit_table_t; > > -static const char szMsgBoxTitle[] = "Wine C++ Runtime Library"; > - > extern int MSVCRT_app_type; > extern MSVCRT_wchar_t *MSVCRT__wpgmptr; > > @@ -116,6 +114,8 @@ static void DoMessageBoxW(const MSVCRT_wchar_t *lead, const MSVCRT_wchar_t *mess > '%','s','\n','\n','P','r','e','s','s',' ','O','K',' ','t','o',' ','e','x','i','t',' ','t','h','e',' ', > 'p','r','o','g','r','a','m',',',' ','o','r',' ','C','a','n','c','e','l',' ','t','o',' ','s','t','a','r','t',' ', > 't','h','e',' ','W','i','n','e',' ','d','e','b','b','u','g','e','r','.','\n',0}; > + static const MSVCRT_wchar_t title[] = > + {'W','i','n','e',' ','C','+','+',' ','R','u','n','t','i','m','e',' ','L','i','b','r','a','r','y',0}; I think this should be WCHAR, because it will be used with regular API. > > MSGBOXPARAMSW msgbox; > MSVCRT_wchar_t text[2048]; > @@ -127,7 +127,7 @@ static void DoMessageBoxW(const MSVCRT_wchar_t *lead, const MSVCRT_wchar_t *mess > msgbox.hwndOwner = GetActiveWindow(); > msgbox.hInstance = 0; > msgbox.lpszText = (LPCWSTR)text; > - msgbox.lpszCaption = (LPCWSTR)szMsgBoxTitle; > + msgbox.lpszCaption = (LPCWSTR)title; Looks like these two casts could be removed. > msgbox.dwStyle = MB_OKCANCEL|MB_ICONERROR; > msgbox.lpszIcon = NULL; > msgbox.dwContextHelpId = 0;

Dec. 22, 2016

Re: [PATCH 2/2] user32: Send WM_NCPAINT and WM_ERASEBKGND when window is shown and add tests

by Fabian Maurer

On Thursday, December 22, 2016 2:17:57 PM CET Marvin wrote: > While running your changed tests on Windows, I think I found new failures. > Being a bot and all I'm not very good at pattern recognition, so I might be > wrong, but could you please double-check? Seems to be the same, I can't confirm these issues. Ran the test again, and they disappeared. Probably not related to my changes.

Dec. 22, 2016

Re: [PATCH 1/2] user32: Fix WM_NCPAINT for windows crossing screen boarders and add tests

by Fabian Maurer

On Thursday, December 22, 2016 2:10:35 PM CET Marvin wrote: > While running your changed tests on Windows, I think I found new failures. > Being a bot and all I'm not very good at pattern recognition, so I might be > wrong, but could you please double-check? Not sure these are related, I couldn't reproduce. Seems to be random, but not by my patches.

Dec. 22, 2016

Re: [PATCH 2/2] user32: Send WM_NCPAINT and WM_ERASEBKGND when window is shown and add tests

by Marvin

Hi, While running your changed tests on Windows, I think I found new failures. Being a bot and all I'm not very good at pattern recognition, so I might be wrong, but could you please double-check? Full results can be found at https://testbot.winehq.org/JobDetails.pl?Key=27468 Your paranoid android. === w7u (32 bit msg) === msg.c:9126: Test failed: did not get zero count as expected (1). msg.c:9129: Test failed: did not get one count as expected (2). msg.c:9133: Test failed: killing replaced timer did not work (2). msg.c:9146: Test failed: did not get expected count for minimum timeout (12 != ~100). === w7pro64 (64 bit msg) === msg.c:12258: Test failed: 56: ShowWindow(SW_SHOWNOACTIVATE): 5: in msg 0x0047 expecting wParam 0x8170 got 0x1803 msg.c:12258: Test failed: 56: ShowWindow(SW_SHOWNOACTIVATE): 6: the msg 0x0003 was expected, but got msg 0x0009 instead msg.c:12258: Test failed: 56: ShowWindow(SW_SHOWNOACTIVATE): 7: the msg 0x0005 was expected, but got msg 0x0047 instead msg.c:12258: Test failed: 56: ShowWindow(SW_SHOWNOACTIVATE): 13: the msg sequence is not complete: expected 0000 - actual 0003

Dec. 22, 2016

Re: [PATCH 1/2] user32: Fix WM_NCPAINT for windows crossing screen boarders and add tests

by Marvin

Hi, While running your changed tests on Windows, I think I found new failures. Being a bot and all I'm not very good at pattern recognition, so I might be wrong, but could you please double-check? Full results can be found at https://testbot.winehq.org/JobDetails.pl?Key=27467 Your paranoid android. === w7pro64 (64 bit msg) === msg.c:12156: Test failed: 54: ShowWindow(SW_MINIMIZE): 4: in msg 0x0047 expecting wParam 0x8170 got 0x1803 msg.c:12156: Test failed: 54: ShowWindow(SW_MINIMIZE): 5: the msg 0x0003 was expected, but got msg 0x0047 instead msg.c:12156: Test failed: 54: ShowWindow(SW_MINIMIZE): 6: the msg 0x0005 was expected, but got msg 0x0003 instead msg.c:12156: Test failed: 54: ShowWindow(SW_MINIMIZE): 7: the msg sequence is not complete: expected 0000 - actual 0005

Dec. 22, 2016

Re: webservices: allocate enough memory for WS_XML_TEXT_TYPE_DOUBLE (Valgrind) (try 2)

by Alexandre Julliard

Hans Leidekker <hans(a)codeweavers.com> writes: > On Wed, 2016-12-21 at 09:38 +0100, Alexandre Julliard wrote: >> Austin English <austinenglish(a)gmail.com> writes: >> >> > On Wed, Dec 14, 2016 at 4:45 PM, Marvin <testbot(a)winehq.org> wrote: >> >> Thank you for your contribution to Wine! >> >> >> >> This is an automated notification to let you know that your patch has >> >> been reviewed and its status set to "Assigned". >> >> >> >> This means that the patch has been assigned to a reviewer. If the >> >> reviewer signs off on the patch, it will be committed, otherwise >> >> you'll receive advice on how to improve it. If you don't hear anything >> >> after a couple of weeks, please resend the patch. >> > >> > This shows in the tracker as assigned to Hans, but Hans signed off a week ago. >> >> Henri objected, and I agree, that we need to understand what's going on, >> and in particular why 48 would be the correct size. > > Valgrind ignores the rounding mode: > > Valgrind, setting RC_DOWN: > writer.c:364: Test failed: 2097: got 39 expected 22 > writer.c:366: Test failed: 2097: got <t>100000000000000.0156249999999999</t> expected <t>100000000000000</t> > > no Valgrind, not setting RC_DOWN: > writer.c:364: Test failed: 2097: got 39 expected 22 > writer.c:366: Test failed: 2097: got <t>100000000000000.0312499999999999</t> expected <t>100000000000000</t> > > 32 characters will be needed at most (excluding XML tags) because the > formatting code switches to exponential notation beyond 1^14 and precision > is capped at 0.0000000000000001, or 16 digits. Thanks for looking into this. Could you please resend a patch with the correct size and an appropriate comment? -- Alexandre Julliard julliard(a)winehq.org

Dec. 21, 2016

Re: webservices: allocate enough memory for WS_XML_TEXT_TYPE_DOUBLE (Valgrind) (try 2)

by Hans Leidekker

On Wed, 2016-12-21 at 09:38 +0100, Alexandre Julliard wrote: > Austin English <austinenglish(a)gmail.com> writes: > > > On Wed, Dec 14, 2016 at 4:45 PM, Marvin <testbot(a)winehq.org> wrote: > >> Thank you for your contribution to Wine! > >> > >> This is an automated notification to let you know that your patch has > >> been reviewed and its status set to "Assigned". > >> > >> This means that the patch has been assigned to a reviewer. If the > >> reviewer signs off on the patch, it will be committed, otherwise > >> you'll receive advice on how to improve it. If you don't hear anything > >> after a couple of weeks, please resend the patch. > > > > This shows in the tracker as assigned to Hans, but Hans signed off a week ago. > > Henri objected, and I agree, that we need to understand what's going on, > and in particular why 48 would be the correct size. Valgrind ignores the rounding mode: Valgrind, setting RC_DOWN: writer.c:364: Test failed: 2097: got 39 expected 22 writer.c:366: Test failed: 2097: got <t>100000000000000.0156249999999999</t> expected <t>100000000000000</t> no Valgrind, not setting RC_DOWN: writer.c:364: Test failed: 2097: got 39 expected 22 writer.c:366: Test failed: 2097: got <t>100000000000000.0312499999999999</t> expected <t>100000000000000</t> 32 characters will be needed at most (excluding XML tags) because the formatting code switches to exponential notation beyond 1^14 and precision is capped at 0.0000000000000001, or 16 digits.

Dec. 21, 2016

Re: webservices: allocate enough memory for WS_XML_TEXT_TYPE_DOUBLE (Valgrind) (try 2)

by Alexandre Julliard

Austin English <austinenglish(a)gmail.com> writes: > On Wed, Dec 14, 2016 at 4:45 PM, Marvin <testbot(a)winehq.org> wrote: >> Thank you for your contribution to Wine! >> >> This is an automated notification to let you know that your patch has >> been reviewed and its status set to "Assigned". >> >> This means that the patch has been assigned to a reviewer. If the >> reviewer signs off on the patch, it will be committed, otherwise >> you'll receive advice on how to improve it. If you don't hear anything >> after a couple of weeks, please resend the patch. > > This shows in the tracker as assigned to Hans, but Hans signed off a week ago. Henri objected, and I agree, that we need to understand what's going on, and in particular why 48 would be the correct size. -- Alexandre Julliard julliard(a)winehq.org

Dec. 21, 2016

Re: webservices: allocate enough memory for WS_XML_TEXT_TYPE_DOUBLE (Valgrind) (try 2)

by Austin English

On Wed, Dec 14, 2016 at 4:45 PM, Marvin <testbot(a)winehq.org> wrote: > Thank you for your contribution to Wine! > > This is an automated notification to let you know that your patch has > been reviewed and its status set to "Assigned". > > This means that the patch has been assigned to a reviewer. If the > reviewer signs off on the patch, it will be committed, otherwise > you'll receive advice on how to improve it. If you don't hear anything > after a couple of weeks, please resend the patch. This shows in the tracker as assigned to Hans, but Hans signed off a week ago. -- -Austin GPG: 14FB D7EA A041 937B

Dec. 20, 2016

Re: Can't build crosstests

by Fabian Maurer

On Tuesday, December 20, 2016 8:28:12 AM CET Andrew Eikum wrote: > I think your "-O0 -rdynamic" options are coming from someplace outside > of configure. Oh yes, you're right... I had a wrong CFLAGS environment variable set. Sorry for that, but thanks for the help!

Dec. 20, 2016

Re: Can't build crosstests

by Andrew Eikum

On Tue, Dec 20, 2016 at 02:37:13PM +0100, Fabian Maurer wrote: > On Tuesday, December 20, 2016 9:00:13 AM CET Jactry Zeng wrote: > > You can have more information about this failure in config.log. > Thanks, I somehow completely missed that... > > The error is: > >configure:8781: i686-w64-mingw32-gcc -c -g -O0 -rdynamic conftest.c >&5 > >i686-w64-mingw32-gcc: error: unrecognized command line option '-rdynamic' > I'm on Arch Linux with mingw-w64-gcc 6.2.1 > Seems like the script passes wrong options to gcc. > I think your "-O0 -rdynamic" options are coming from someplace outside of configure. I'm also on Arch Linux and I see: configure:8924: result: ccache i686-w64-mingw32-gcc configure:8940: checking whether ccache i686-w64-mingw32-gcc works configure:8953: ccache i686-w64-mingw32-gcc -c -g -O2 conftest.c >&5 I also noticed the configure line numbers are different, which seems strange as configure is tracked by git. The string 'rdynamic' doesn't appear in Wine's configure script. Andrew

Dec. 20, 2016

Re: Can't build crosstests

by Fabian Maurer

On Tuesday, December 20, 2016 9:00:13 AM CET Jactry Zeng wrote: > You can have more information about this failure in config.log. Thanks, I somehow completely missed that... The error is: >configure:8781: i686-w64-mingw32-gcc -c -g -O0 -rdynamic conftest.c >&5 >i686-w64-mingw32-gcc: error: unrecognized command line option '-rdynamic' I'm on Arch Linux with mingw-w64-gcc 6.2.1 Seems like the script passes wrong options to gcc.

Dec. 20, 2016

Re: user32/tests: Disable automatic shell restart during SetShellWindow tests.

by Hans Leidekker

On Tue, 2016-12-20 at 05:31 -0600, Marvin wrote: > === w864 (32 bit win) === > win.c:2305: Test failed: expected !100 > win.c:2305: Test failed: expected !100 > > === w864 (64 bit win) === > win.c:2305: Test failed: expected !100 > win.c:2305: Test failed: expected !100 This failure is also seen with a whitespace patch: https://testbot.winehq.org/JobDetails.pl?Key=27439

Dec. 20, 2016

Re: user32/tests: Disable automatic shell restart during SetShellWindow tests.

by Marvin

Hi, While running your changed tests on Windows, I think I found new failures. Being a bot and all I'm not very good at pattern recognition, so I might be wrong, but could you please double-check? Full results can be found at https://testbot.winehq.org/JobDetails.pl?Key=27438 Your paranoid android. === w864 (32 bit win) === win.c:2305: Test failed: expected !100 win.c:2305: Test failed: expected !100 === w864 (64 bit win) === win.c:2305: Test failed: expected !100 win.c:2305: Test failed: expected !100

Dec. 20, 2016

Re: Can't build crosstests

by Jactry Zeng

Hi Fabian, You can have more information about this failure in config.log. 2016-12-20 8:56 GMT+08:00 Fabian Maurer <dark.shadow4(a)web.de>: > Hello, > > I'm trying to build the tests for windows, but I can't get it working > anymore. > I have mingw installed and it does work for cross-compiling with > code-blocks, > but wine says it's broken. > The configure script fails with > >checking for i686-w64-mingw32-gcc... i686-w64-mingw32-gcc > >checking whether i686-w64-mingw32-gcc works... no > > An idea how to find out what's going wrong? > > > -- Regards, Jactry Zeng

Dec. 20, 2016

Can't build crosstests

by Fabian Maurer

Hello, I'm trying to build the tests for windows, but I can't get it working anymore. I have mingw installed and it does work for cross-compiling with code-blocks, but wine says it's broken. The configure script fails with >checking for i686-w64-mingw32-gcc... i686-w64-mingw32-gcc >checking whether i686-w64-mingw32-gcc works... no An idea how to find out what's going wrong?

Dec. 20, 2016

Re: [PATCH] gdiplus/tests: Test for initial properties of adjustable arrow caps

by Nikolay Sivov

On 19.12.2016 20:46, Vincent Povirk wrote: > On Fri, Dec 16, 2016 at 6:21 AM, Nikolay Sivov <nsivov(a)codeweavers.com> wrote: >> + stat = GdipGetCustomLineCapBaseInset((GpCustomLineCap*)cap, &inset); >> + ok(stat == Ok, "Unexpected return code, %d\n", stat); >> + > It isn't enough to reject the patch, but I'm curious why you don't > test this value. > > I only tested this briefly, inset depends on width/height ratio. It felt like one test is not enough, so I decided to leave it to come back later. That of course means I can remove this call altogether.

Dec. 19, 2016

Re: [PATCH] gdiplus/tests: Test for initial properties of adjustable arrow caps

by Vincent Povirk

On Fri, Dec 16, 2016 at 6:21 AM, Nikolay Sivov <nsivov(a)codeweavers.com> wrote: > + stat = GdipGetCustomLineCapBaseInset((GpCustomLineCap*)cap, &inset); > + ok(stat == Ok, "Unexpected return code, %d\n", stat); > + It isn't enough to reject the patch, but I'm curious why you don't test this value.

Dec. 19, 2016

Re: shell32/tests: Skip SHFileOperationW tests if the user doesn't have admin rights.

by Hans Leidekker

On Mon, 2016-12-19 at 14:38 +0300, Nikolay Sivov wrote: > > diff --git a/dlls/shell32/tests/shlfileop.c b/dlls/shell32/tests/shlfileop.c > > index 0e502fc..776da98 100644 > > --- a/dlls/shell32/tests/shlfileop.c > > +++ b/dlls/shell32/tests/shlfileop.c > > @@ -819,7 +819,7 @@ static void test_rename(void) > > retval == DE_DIFFDIR || /* Vista */ > > broken(retval == DE_OPCANCELLED) || /* Win9x */ > > broken(retval == 65652), /* NT4 */ > > - "Expected ERROR_CANCELLED or DE_DIFFDIR\n"); > > + "Expected ERROR_CANCELLED or DE_DIFFDIR, got %u\n", retval); > > ok(file_exists("test1.txt"), "Expected test1.txt to exist\n"); > > > > /* pFrom is empty */ > > @@ -2516,6 +2516,11 @@ static void test_unicode(void) > > skip("Unicode tests skipped on non-unicode system\n"); > > return; > > } > > + if (GetLastError()==ERROR_ACCESS_DENIED) > > + { > > + skip("test needs admin rights\n"); > > + return; > > + } > > CloseHandle(file); > > > > /* Try to delete a file with unicode filename */ > > @@ -2574,6 +2579,7 @@ static void test_unicode(void) > > ok(GetLastError() == ERROR_SUCCESS || > > broken(GetLastError() == ERROR_INVALID_HANDLE), /* WinXp, win2k3 */ > > "Expected ERROR_SUCCESS, got %d\n", GetLastError()); > > + DeleteFileW(UNICODE_PATH_TO); > > > > /* Check last error after a failed file operation. */ > > DeleteFileW(UNICODE_PATH); > Will it work if user temp directory was used instead of current > directory or C: root? Yes, running it from a temporary directory instead of c: makes the failure go away.

Dec. 19, 2016

Re: shell32/tests: Skip SHFileOperationW tests if the user doesn't have admin rights.

by Nikolay Sivov

On 19.12.2016 13:38, Hans Leidekker wrote: > Signed-off-by: Hans Leidekker <hans(a)codeweavers.com> > --- > dlls/shell32/tests/shlfileop.c | 8 +++++++- > 1 file changed, 7 insertions(+), 1 deletion(-) > > diff --git a/dlls/shell32/tests/shlfileop.c b/dlls/shell32/tests/shlfileop.c > index 0e502fc..776da98 100644 > --- a/dlls/shell32/tests/shlfileop.c > +++ b/dlls/shell32/tests/shlfileop.c > @@ -819,7 +819,7 @@ static void test_rename(void) > retval == DE_DIFFDIR || /* Vista */ > broken(retval == DE_OPCANCELLED) || /* Win9x */ > broken(retval == 65652), /* NT4 */ > - "Expected ERROR_CANCELLED or DE_DIFFDIR\n"); > + "Expected ERROR_CANCELLED or DE_DIFFDIR, got %u\n", retval); > ok(file_exists("test1.txt"), "Expected test1.txt to exist\n"); > > /* pFrom is empty */ > @@ -2516,6 +2516,11 @@ static void test_unicode(void) > skip("Unicode tests skipped on non-unicode system\n"); > return; > } > + if (GetLastError()==ERROR_ACCESS_DENIED) > + { > + skip("test needs admin rights\n"); > + return; > + } > CloseHandle(file); > > /* Try to delete a file with unicode filename */ > @@ -2574,6 +2579,7 @@ static void test_unicode(void) > ok(GetLastError() == ERROR_SUCCESS || > broken(GetLastError() == ERROR_INVALID_HANDLE), /* WinXp, win2k3 */ > "Expected ERROR_SUCCESS, got %d\n", GetLastError()); > + DeleteFileW(UNICODE_PATH_TO); > > /* Check last error after a failed file operation. */ > DeleteFileW(UNICODE_PATH); Will it work if user temp directory was used instead of current directory or C: root?

Dec. 19, 2016

Re: shell32/tests: Skip SHFileOperationW tests if the user doesn't have admin rights.

by Hans Leidekker

On Mon, 2016-12-19 at 04:54 -0600, Marvin wrote: > Hi, > > While running your changed tests on Windows, I think I found new failures. > Being a bot and all I'm not very good at pattern recognition, so I might be > wrong, but could you please double-check? > Full results can be found at > https://testbot.winehq.org/JobDetails.pl?Key=27406 > > Your paranoid android. > > > === wvistau64 (32 bit shlfileop) === > shlfileop.c:818: Test failed: Expected ERROR_CANCELLED or DE_DIFFDIR, got 128 > > === wvistau64 (64 bit shlfileop) === > shlfileop.c:818: Test failed: Expected ERROR_CANCELLED or DE_DIFFDIR, got 128 This test also fails with a whitespace patch: https://testbot.winehq.org/JobDetails.pl?Key=27409

Dec. 19, 2016

Re: shell32/tests: Skip SHFileOperationW tests if the user doesn't have admin rights.

by Marvin

Hi, While running your changed tests on Windows, I think I found new failures. Being a bot and all I'm not very good at pattern recognition, so I might be wrong, but could you please double-check? Full results can be found at https://testbot.winehq.org/JobDetails.pl?Key=27406 Your paranoid android. === wvistau64 (32 bit shlfileop) === shlfileop.c:818: Test failed: Expected ERROR_CANCELLED or DE_DIFFDIR, got 128 === wvistau64 (64 bit shlfileop) === shlfileop.c:818: Test failed: Expected ERROR_CANCELLED or DE_DIFFDIR, got 128

Dec. 19, 2016

Re: [PATCH] webservices: free the potentially allocated pointer (Coverity)

by Marcus Meissner

On Mon, Dec 19, 2016 at 09:14:16AM +0100, Hans Leidekker wrote: > On Sat, 2016-12-17 at 12:48 +0100, Marcus Meissner wrote: > > 1374356 Resource leak > > > > Signed-off-by: Marcus Meissner <marcus(a)jet.franken.de> > > --- > > dlls/webservices/reader.c | 26 ++++++++++++++++++++++---- > > 1 file changed, 22 insertions(+), 4 deletions(-) > > > > diff --git a/dlls/webservices/reader.c b/dlls/webservices/reader.c > > index d67c47a701..219683b2af 100644 > > --- a/dlls/webservices/reader.c > > +++ b/dlls/webservices/reader.c > > @@ -4701,15 +4701,33 @@ static ULONG get_field_size( const WS_FIELD_DESCRIPTION *desc ) > > > > static HRESULT read_param( struct reader *reader, const WS_FIELD_DESCRIPTION *desc, WS_HEAP *heap, void *ret ) > > { > > - if (!ret && !(ret = ws_alloc_zero( heap, get_field_size(desc) ))) return WS_E_QUOTA_EXCEEDED; > > - return read_type_struct_field( reader, desc, heap, ret, 0 ); > > + HRESULT hr; > > + VOID *xret = NULL; > > + > > + if (!ret) > > + { > > + if (!(xret = ws_alloc_zero( heap, get_field_size(desc) ))) return WS_E_QUOTA_EXCEEDED; > > + ret = xret; > > + } > > + hr = read_type_struct_field( reader, desc, heap, ret, 0 ); > > + ws_free( heap, xret); > > + return hr; > > } > > It's not a leak. The memory is allocated on the heap passed to this function and it will > be released when the heap is destroyed. I guess Coverity doesn't see that. Yes, it does not think in Heaps. Will mark the issue as invalid. Ciao, Marcus

Dec. 19, 2016

Re: [PATCH 2/4] ddraw: Subtract framebuffer size from reported available video memory.

by Henri Verbeet

On 18 December 2016 at 00:22, Józef Kucia <jkucia(a)codeweavers.com> wrote: > + INT64 total_vidmem, free_vidmem; ... > + free_vidmem = wined3d_device_get_available_texture_mem(ddraw->wined3d_device); > + *free = max(0, free_vidmem - framebuffer_size); This works, but it seems a little silly to use INT64 to avoid writing something along the lines of "*free = framebuffer_size > free_vidmem ? 0 : free_vidmem - framebuffer_size;".

Dec. 19, 2016

Re: [PATCH] webservices: free the potentially allocated pointer (Coverity)

by Hans Leidekker

On Sat, 2016-12-17 at 12:48 +0100, Marcus Meissner wrote: > 1374356 Resource leak > > Signed-off-by: Marcus Meissner <marcus(a)jet.franken.de> > --- > dlls/webservices/reader.c | 26 ++++++++++++++++++++++---- > 1 file changed, 22 insertions(+), 4 deletions(-) > > diff --git a/dlls/webservices/reader.c b/dlls/webservices/reader.c > index d67c47a701..219683b2af 100644 > --- a/dlls/webservices/reader.c > +++ b/dlls/webservices/reader.c > @@ -4701,15 +4701,33 @@ static ULONG get_field_size( const WS_FIELD_DESCRIPTION *desc ) > > static HRESULT read_param( struct reader *reader, const WS_FIELD_DESCRIPTION *desc, WS_HEAP *heap, void *ret ) > { > - if (!ret && !(ret = ws_alloc_zero( heap, get_field_size(desc) ))) return WS_E_QUOTA_EXCEEDED; > - return read_type_struct_field( reader, desc, heap, ret, 0 ); > + HRESULT hr; > + VOID *xret = NULL; > + > + if (!ret) > + { > + if (!(xret = ws_alloc_zero( heap, get_field_size(desc) ))) return WS_E_QUOTA_EXCEEDED; > + ret = xret; > + } > + hr = read_type_struct_field( reader, desc, heap, ret, 0 ); > + ws_free( heap, xret); > + return hr; > } It's not a leak. The memory is allocated on the heap passed to this function and it will be released when the heap is destroyed. I guess Coverity doesn't see that.

Dec. 19, 2016

Re: Wined3d textureless rendering

by Bruno Jesus

On Wed, Dec 14, 2016 at 11:59 AM, Henri Verbeet <hverbeet(a)gmail.com> wrote: > On 14 December 2016 at 14:31, Bruno Jesus <00cpxxx(a)gmail.com> wrote: >> Hello everyone. Following my email last year email asking if rendering >> in wireframe could speedup a game (answer was no in many different >> ways) I have a new idea after playing Totaly Accurate Battle Simulator >> with its red and blue army. >> >> What if I skip applying every texture to every object? Probably >> everything would be black so I would need to use different incremental >> solid color to distinguish stuff in the screen. Would that save memory >> and also make games faster? >> > Probably, in some cases, depending on where the performance bottleneck > actually is. In fact, that's a performance analysis technique you can > use to figure out if your (shader) performance is limited by texture > lookups or by something else. > > That doesn't mean you can generally apply that to make things go > faster though. Textures are used in a lot of places, sometimes > unexpected ones. Think for example about text in the user interface, > HUD elements, etc. If you want to experiment a bit, fixed function > texture lookups happen in shader_glsl_generate_ffp_fragment_shader(), > for shaders you'll want to look at anything that calls > shader_glsl_get_sample_function(), perhaps most notably > shader_glsl_tex(). Thanks! I'm trying and having "interesting" experiences.

Dec. 18, 2016

[OT] Machine learning for automatical GUI testing

by Qian Hong

Hi folks, I was falling into machine learning recently and I think I'd like to share some ideas. Here is one of them: quote: ``` Universe allows an AI agent to use a computer like a human does: by looking at screen pixels and operating a virtual keyboard and mouse. We must train AI systems on the full range of tasks we expect them to solve, and Universe lets us train a single agent on any task a human can complete with a computer. ``` Reference: https://openai.com/blog/universe/ Reinforcement Learning is the powerful algorithm used by AlphaGo which won the Go chess game this year against world class master. OpenAI Universe is a good environment to train Reinforcement Learning program's skill on software UI testing. We might want to follow the latest progress from OpenAI Universe community to see if there is anything useful for Wine development / testing. I know people tried GUI auto testing years ago but was not very happy with that, however this time with the revolution of machine learning, interesting thing could happen, finally. Any one want to discover more potential on using machine learning to improve Wine? This might not be practical in one month, but could be very powerful in long term, like 3 years or more. There could be multiple possible ways, not only GUI testing, but none is easy today. Other interesting reading: https://openai.com/blog/universe/ https://gym.openai.com/ Cheers! -- Regards, Qian Hong

Dec. 18, 2016

Re: [Tools] testbot: Skip ntdll:exception on the Windows 10 VM.

by Francois Gouget

On Fri, 16 Dec 2016, Francois Gouget wrote: [...] > - $Script .= "-q -o $RptFileName -t $Tag -m \"$EMail\" -i \"$Info\"\r\n" . > + my $Ignore = ($VM->Name eq "w1064" ? "-n ntdll:exception" : ""); > + $Script .= "-q -o $RptFileName -t $Tag -m \"$EMail\" -i \"$Info\" $Ignore\r\n" . > "$FileName -q -s $RptFileName\r\n"; This should not be needed after all. The trick is not to upgrade QEMU, Windows 10, the host kernel, change the emulated CPU or switch to emulating USB 3. The trick is: echo 1 >/sys/module/kvm/parameters/ignore_msrs Once you've noticed the following lines in the host's kernel log and done a Google search: Dec 16 13:43:48 vm3 kernel: [ 191.624802] kvm [2064]: vcpu0, guest rIP: 0xfffff803cb3c0bf3 kvm_set_msr_common: MSR_IA32_DEBUGCTLMSR 0x1, nop Dec 16 13:43:48 vm3 kernel: [ 191.624835] kvm [2064]: vcpu0, guest rIP: 0xfffff803cb3c0c5c unhandled rdmsr: 0x1c9 https://www.reddit.com/r/VFIO/comments/42dj7n/some_games_crash_to_biosboot_… Now as to why another Windows 10 VM seemed to get magically fixed by the AEMU upgrade (from 2.1 to 2.7) and did not require this hack is another mystery. But one I can live with. -- Francois Gouget <fgouget(a)codeweavers.com>

Dec. 16, 2016

Re: [PATCH 3/7] d3d11/tests: Add workaround for bug in AMD Windows driver.

by Józef Kucia

On Fri, Dec 16, 2016 at 11:11 AM, Józef Kucia <joseph.kucia(a)gmail.com> wrote: > On Fri, Dec 16, 2016 at 10:53 AM, Henri Verbeet <hverbeet(a)gmail.com> wrote: >> On 15 December 2016 at 13:45, Józef Kucia <jkucia(a)codeweavers.com> wrote: >>> Signed-off-by: Józef Kucia <jkucia(a)codeweavers.com> >>> --- >>> dlls/d3d11/tests/d3d11.c | 59 +++++++++++++++++++++++++++--------------------- >>> 1 file changed, 33 insertions(+), 26 deletions(-) >>> >> Unfortunately this makes it fail on Wine for me, probably because the >> initial texture data is undefined. > > I think the initial texture data should be initialized. The texture > accessed from SRV uses non-depth/stencil format and CopyResource() > should do nothing when one texture is depth/stencil and other isn't. > I'll take a second look. It fails because it depends on undefined behavior in OpenGL. We use a non-depth stencil format (e.g. GL_R16, GL_R32) when we create a texture and D3D11_BIND_DEPTH_STENCIL is not set. In the test such texture is used with a shadow sampler. It works with most drivers, but fails with radeonsi. I am currently in favor of keeping the test unmodified. We can just skip it on Windows with AMD GPUs (see is_amd_device() in patch 128549). Of course, if we find an application which uses non-depth/stencil textures with comparison samplers, we'll have to fix the underlying issue.

Dec. 16, 2016

Re: [PATCH 3/7] d3d11/tests: Add workaround for bug in AMD Windows driver.

by Józef Kucia

On Fri, Dec 16, 2016 at 10:53 AM, Henri Verbeet <hverbeet(a)gmail.com> wrote: > On 15 December 2016 at 13:45, Józef Kucia <jkucia(a)codeweavers.com> wrote: >> Signed-off-by: Józef Kucia <jkucia(a)codeweavers.com> >> --- >> dlls/d3d11/tests/d3d11.c | 59 +++++++++++++++++++++++++++--------------------- >> 1 file changed, 33 insertions(+), 26 deletions(-) >> > Unfortunately this makes it fail on Wine for me, probably because the > initial texture data is undefined. I think the initial texture data should be initialized. The texture accessed from SRV uses non-depth/stencil format and CopyResource() should do nothing when one texture is depth/stencil and other isn't. I'll take a second look.

Dec. 16, 2016

Re: [PATCH 7/7] wined3d: Make total video memory limit optional.

by Henri Verbeet

On 16 December 2016 at 10:51, Józef Kucia <joseph.kucia(a)gmail.com> wrote: > Possibly yes, but then memory accounting would be inconsistent. Not necessarily, ddraw7_GetAvailableVidMem() exposes both total and free memory, and is the only place in ddraw that exposes those. There's perhaps an argument that it would allow more resources to be created than would fit in the reported total available video memory, but I don't think that's actually true. The documentation from GetAvailableVidMem() suggests to me that memory for the primary surface is just not counted with the total. > There > is also a consideration if the reported memory amount should change > when the application switches the display mode. When it comes to > whether this should be version specific I think we should enable this > workaround only for older Windows versions. It's important to note > that these applications aren't expected to work on modern Windows > because GetAvailableVidMem() reports more memory than INT_MAX. It's > just a workaround to make these buggy applications to work in Wine. The limit to INT_MAX, yes, and I wouldn't necessarily expect that to be specific to ddraw. Subtracting the memory for the primary surface from the total on the other hand looks like the way ddraw is intended to work to me, regardless of the Windows version.

Dec. 16, 2016

Re: [PATCH 3/7] d3d11/tests: Add workaround for bug in AMD Windows driver.

by Henri Verbeet

On 15 December 2016 at 13:45, Józef Kucia <jkucia(a)codeweavers.com> wrote: > Signed-off-by: Józef Kucia <jkucia(a)codeweavers.com> > --- > dlls/d3d11/tests/d3d11.c | 59 +++++++++++++++++++++++++++--------------------- > 1 file changed, 33 insertions(+), 26 deletions(-) > Unfortunately this makes it fail on Wine for me, probably because the initial texture data is undefined.

Dec. 16, 2016

Re: [PATCH 7/7] wined3d: Make total video memory limit optional.

by Józef Kucia

On Fri, Dec 16, 2016 at 10:13 AM, Henri Verbeet <hverbeet(a)gmail.com> wrote: > On 15 December 2016 at 13:45, Józef Kucia <jkucia(a)codeweavers.com> wrote: >> It's enough to enable this workaround for ddraw. >> > So would it then also be enough to just fixup things in > ddraw7_GetAvailableVidMem()? In fact, the documentation suggests > that's actually the proper place, and that this shouldn't be specific > to the Windows version. You could probably use > wined3d_get_adapter_display_mode() together with > wined3d_calculate_format_pitch() to calculate the primary surface > size. Possibly yes, but then memory accounting would be inconsistent. There is also a consideration if the reported memory amount should change when the application switches the display mode. When it comes to whether this should be version specific I think we should enable this workaround only for older Windows versions. It's important to note that these applications aren't expected to work on modern Windows because GetAvailableVidMem() reports more memory than INT_MAX. It's just a workaround to make these buggy applications to work in Wine.

Dec. 16, 2016

Re: [PATCH 7/7] wined3d: Make total video memory limit optional.

by Henri Verbeet

On 15 December 2016 at 13:45, Józef Kucia <jkucia(a)codeweavers.com> wrote: > It's enough to enable this workaround for ddraw. > So would it then also be enough to just fixup things in ddraw7_GetAvailableVidMem()? In fact, the documentation suggests that's actually the proper place, and that this shouldn't be specific to the Windows version. You could probably use wined3d_get_adapter_display_mode() together with wined3d_calculate_format_pitch() to calculate the primary surface size.

Dec. 16, 2016

Re: [PATCH v2 14/18] d3d11/tests: Add test for depth/stencil shader resource views.

by Matteo Bruni

2016-12-14 9:49 GMT+01:00 Henri Verbeet <hverbeet(a)gmail.com>: > On 13 December 2016 at 13:21, Józef Kucia <joseph.kucia(a)gmail.com> wrote: >> I expect that the attached patch fixes the test failures. >> Unfortunately it breaks the test on Wine, because blit doesn't work >> between depth/stencil R32_TYPELESS and non-detph/stencil R32_TYPELESS. > > That does fix it, yeah, and fails on Wine as expected. > > The failure on Wine is unfortunate, but perhaps also an interesting > case by itself. Yeah, I don't know that is something actual applications are likely to use but it's probably a valuable test.

Dec. 15, 2016

Re: [PATCH] d3dx9: Fix a typo in TRACE() message.

by Matteo Bruni

2016-12-12 11:21 GMT+01:00 Andrey Gusev <andrey.goosev(a)gmail.com>: > @@ -750,7 +750,7 @@ D3DXMATRIX* WINAPI D3DXMatrixTransformation(D3DXMATRIX *pout, const D3DXVECTOR3 > D3DXQUATERNION prc; > D3DXVECTOR3 psc, pt; > > - TRACE("pout %p, pscalingcenter %p, pscalingrotation %p, pscaling %p, protationcentr %p, protation %p, ptranslation %p\n", > + TRACE("pout %p, pscalingcenter %p, pscalingrotation %p, pscaling %p, protationcenter %p, protation %p, ptranslation %p\n", > pout, pscalingcenter, pscalingrotation, pscaling, protationcenter, protation, ptranslation); > > if ( !pscalingcenter ) It would be nice to clean up the function while at it (add the missing period at the end of the trace, drop the useless 'p' prefix from all the parameters and most of the local variables, fix and split the function prototype over multiple lines, remove the spaces around the if condition, ...) IMO better if you send a v2 with the cleanups after Wine 2.0.

Dec. 15, 2016

Re: [PATCH v2 1/3] msvcrt/tests: Test wcstoi64 with various Unicode digits.

by Piotr Caban

On 12/15/16 16:22, Lauri Kenttä wrote: > Should I just send v3 after 2.0 is out? Yes. Thank you, Piotr

Dec. 15, 2016

Re: [PATCH v2 1/3] msvcrt/tests: Test wcstoi64 with various Unicode digits.

by Lauri Kenttä

On 2016-12-15 13:01, Piotr Caban wrote: > Hi, > > On 12/14/16 18:18, Lauri Kenttä wrote: >> + for (i = 0; i < sizeof(zeros) / sizeof(zeros[0]); ++i) { >> + WCHAR tmp[] = {zeros[i] + 4, zeros[i], zeros[i] + 5, 0}; >> + res = p_wcstoi64(tmp, NULL, 0); >> + todo_wine ok(res == 405, "with zero = U+%04X: got %d, >> expected 405\n", zeros[i], (int)res); >> + tmp[1] = zeros[i] + 10; >> + res = p_wcstoi64(tmp, NULL, 0); > Please change the line to: > res = p_wcstoi64(tmp, NULL, 16); > Currently you don't test if zeros[i]+10 is recognized as valid digit. Right. > Except of that the patches are looking good for me. Are these patches > fixing any bugs? Is any application working thanks to them (I'm asking > because of the code freeze)? I'd assume that at least someone uses these functions, but I don't know about any specific apps. Should I just send v3 after 2.0 is out? -- Lauri Kenttä

Dec. 15, 2016

Re: [PATCH] appwiz.cpl: Remove list entries left over from an earlier uninstall

by Hugh McMaster

On Thursday, 15 December 2016 11:47 PM, Dmitry Timoshkov wrote: > Hugh McMaster wrote: >> -IMPORTS = uuid urlmon advpack comctl32 advapi32 shell32 ole32 user32 comdlg32 >> +IMPORTS = uuid urlmon advpack comctl32 advapi32 shell32 ole32 user32 comdlg32 shlwapi >... >> +#include <shlwapi.h> >... >> - info->regroot = root; >> + info->regroot = SHRegDuplicateHKey(root); > > Adding a whole new import just because of a simple RegOpenKeyEx() wrapper > is not really justified. I never thought to check what SHRegDuplicateHKey() actually did. Thanks for the feedback. I'll send an updated patch tomorrow. -- Hugh

Dec. 15, 2016

Re: [PATCH] appwiz.cpl: Remove list entries left over from an earlier uninstall

by Dmitry Timoshkov

Hugh McMaster <hugh.mcmaster(a)outlook.com> wrote: > -IMPORTS = uuid urlmon advpack comctl32 advapi32 shell32 ole32 user32 comdlg32 > +IMPORTS = uuid urlmon advpack comctl32 advapi32 shell32 ole32 user32 comdlg32 shlwapi ... > +#include <shlwapi.h> ... > - info->regroot = root; > + info->regroot = SHRegDuplicateHKey(root); Adding a whole new import just because of a simple RegOpenKeyEx() wrapper is not really justified. -- Dmitry.

Dec. 15, 2016

Re: [PATCH v2 1/3] msvcrt/tests: Test wcstoi64 with various Unicode digits.

by Piotr Caban

Hi, On 12/14/16 18:18, Lauri Kenttä wrote: > + for (i = 0; i < sizeof(zeros) / sizeof(zeros[0]); ++i) { > + WCHAR tmp[] = {zeros[i] + 4, zeros[i], zeros[i] + 5, 0}; > + res = p_wcstoi64(tmp, NULL, 0); > + todo_wine ok(res == 405, "with zero = U+%04X: got %d, expected 405\n", zeros[i], (int)res); > + tmp[1] = zeros[i] + 10; > + res = p_wcstoi64(tmp, NULL, 0); Please change the line to: res = p_wcstoi64(tmp, NULL, 16); Currently you don't test if zeros[i]+10 is recognized as valid digit. Except of that the patches are looking good for me. Are these patches fixing any bugs? Is any application working thanks to them (I'm asking because of the code freeze)? Thank you, Piotr

Dec. 15, 2016

Re: [v3] winex11: Use XIAllDevices to select for XI_RawMotion

by Carlos Garnacho

Hi Alexandre, On Thu, Dec 15, 2016 at 8:26 AM, Alexandre Julliard <julliard(a)winehq.org> wrote: > Carlos Garnacho <carlosg(a)gnome.org> writes: > >> Hi all, >> >> Got this message from Marvin about "formatting", but I'm not sure what >> could have triggered this. Getting picky I see the parens at the >> update_relative_valuators function aren't alright given surrounding >> code style, but I see nothing specifically against >> https://wiki.winehq.org/Submitting_Patches#Code_guidelines or >> https://source.winehq.org/patches/#legend >> >> Is there anything actually wrong? I'll be happy to send an updated patch. > > Sorry, the "formatting" status was my mistake. It was meant to be marked > "deferred", as in being too large to go in during code freeze. Oh, sure, that's perfectly understandable. Congrats for the ongoing 2.0 release! Cheers, Carlos

Dec. 15, 2016

Re: [v3] winex11: Use XIAllDevices to select for XI_RawMotion

by Alexandre Julliard

Carlos Garnacho <carlosg(a)gnome.org> writes: > Hi all, > > Got this message from Marvin about "formatting", but I'm not sure what > could have triggered this. Getting picky I see the parens at the > update_relative_valuators function aren't alright given surrounding > code style, but I see nothing specifically against > https://wiki.winehq.org/Submitting_Patches#Code_guidelines or > https://source.winehq.org/patches/#legend > > Is there anything actually wrong? I'll be happy to send an updated patch. Sorry, the "formatting" status was my mistake. It was meant to be marked "deferred", as in being too large to go in during code freeze. -- Alexandre Julliard julliard(a)winehq.org

Dec. 15, 2016

Re: wine-staging status

by Michael Müller

Am 14.12.2016 um 18:44 schrieb Erich E. Hoover: > Sebastian is bringing up a new build server at the moment so that we > can handle more automated builds (this also affects devel and stable). > He's hoping to push a new release out as soon as the new build server > is up and running. As Erich already said, we are currently working on some infrastructural changes and therefore delayed the 2.0-rc1 release. We chose this time since most Wine Staging users are interested in bleeding edge features and they don't miss much during the code freeze ;-). We didn't intend to skip the last release though, it was just a bad coincidence as Sebastian and I got sick. I am pretty confident that we can release 2.0 rc1 soon. Regards, Michael

Dec. 14, 2016

Re: wine-staging status

by Erich E. Hoover

On Wed, Dec 14, 2016 at 10:08 AM, Michael Cronenworth <mike(a)cchtml.com> wrote: > Hi, > > Is Wine Staging on hiatus? MIA? DIA? It has been one month since the last > release with no signs of any new commits. I sent an e-mail to Sebastian but > no response yet. Sebastian is bringing up a new build server at the moment so that we can handle more automated builds (this also affects devel and stable). He's hoping to push a new release out as soon as the new build server is up and running. Best, Erich

Dec. 14, 2016

Re: [PATCH 3/3] msvcrt: Make wcstoi64 support various Unicode digits.

by Lauri Kenttä

On 2016-12-14 18:36, Henri Verbeet wrote: > On 14 December 2016 at 17:27, Lauri Kenttä <lauri.kentta(a)gmail.com> > wrote: >> Unfortunately MAP_FOLDDIGITS seems to map too many things (e.g. Tamil, >> which is already tested and shouldn't work). >> > Does it on Windows as well? That is, our unicode tables aren't > necessarily quite the same as the Microsoft ones, and the Microsoft > ones aren't necessarily the same across Windows versions. Likewise, is > that consistent across different versions of msvcr? I could imagine > different msvcr versions matching different unicode versions. Testing on Windows Server 2003, I get 84 code points which wcstoi64 doesn't recognize but MAP_FOLDDIGITS manages to convert. These include superscripts, subscripts, circled numbers etc., and Tamil. The most interesting question is, what's wrong with Tamil? :( Maybe we could use MAP_FOLDDIGITS anyway, but bug for bug, right? And as for different Windows versions, my wctoint seems to be fine: https://testbot.winehq.org/JobDetails.pl?Key=27358 -- Lauri Kenttä

Dec. 14, 2016

wine-staging status

by Michael Cronenworth

Hi, Is Wine Staging on hiatus? MIA? DIA? It has been one month since the last release with no signs of any new commits. I sent an e-mail to Sebastian but no response yet. Thanks, Michael

Dec. 14, 2016

Re: [PATCH 3/3] msvcrt: Make wcstoi64 support various Unicode digits.

by Henri Verbeet

On 14 December 2016 at 17:27, Lauri Kenttä <lauri.kentta(a)gmail.com> wrote: > Unfortunately MAP_FOLDDIGITS seems to map too many things (e.g. Tamil, > which is already tested and shouldn't work). > Does it on Windows as well? That is, our unicode tables aren't necessarily quite the same as the Microsoft ones, and the Microsoft ones aren't necessarily the same across Windows versions. Likewise, is that consistent across different versions of msvcr? I could imagine different msvcr versions matching different unicode versions.

Dec. 14, 2016

Re: [PATCH 3/3] msvcrt: Make wcstoi64 support various Unicode digits.

by Lauri Kenttä

On 2016-12-14 16:59, Henri Verbeet wrote: > On 14 December 2016 at 15:54, Nikolay Sivov <nsivov(a)codeweavers.com> > wrote: >> On 12/14/2016 05:45 PM, Lauri Kenttä wrote: >>> On 2016-12-14 12:23, Henri Verbeet wrote: >>>> For what it's worth, note also that Wine has wine_fold_string(), >>>> which >>>> should be consistent with FoldString(). >>> >>> >>> I don't see what FoldString could do here. >> >> MAP_FOLDDIGITS looks relevant to what you're doing. >> > Yeah. MAP_FOLDDIGITS will map the various unicode digits to 0-9. It > does that based on the unicode tables, which means you wouldn't have > to maintain a separate list of unicode digit ranges in msvcrt. That > assumes wcstoi64() is consistent with FoldString(), which of course it > may not be. Unfortunately MAP_FOLDDIGITS seems to map too many things (e.g. Tamil, which is already tested and shouldn't work). I've now verified the list of zeros with the obvious test program: WCHAR buf[4] = {0}, *end; int i, nl = 0; for (buf[0] = '0'; buf[0] != 0; ++buf[0]) { end = buf; i = _wcstoi64(buf, &end, 36); if (end != buf) { printf("U+%04X = %d\n", buf[0], i); } } There's no point in including this in my patch, though, I guess. -- Lauri Kenttä

Dec. 14, 2016

Re: [PATCH 3/3] msvcrt: Make wcstoi64 support various Unicode digits.

by Henri Verbeet

On 14 December 2016 at 15:54, Nikolay Sivov <nsivov(a)codeweavers.com> wrote: > On 12/14/2016 05:45 PM, Lauri Kenttä wrote: >> On 2016-12-14 12:23, Henri Verbeet wrote: >>> For what it's worth, note also that Wine has wine_fold_string(), which >>> should be consistent with FoldString(). >> >> >> I don't see what FoldString could do here. > > MAP_FOLDDIGITS looks relevant to what you're doing. > Yeah. MAP_FOLDDIGITS will map the various unicode digits to 0-9. It does that based on the unicode tables, which means you wouldn't have to maintain a separate list of unicode digit ranges in msvcrt. That assumes wcstoi64() is consistent with FoldString(), which of course it may not be.

Dec. 14, 2016

Re: [PATCH 3/3] msvcrt: Make wcstoi64 support various Unicode digits.

by Nikolay Sivov

On 12/14/2016 05:45 PM, Lauri Kenttä wrote: > On 2016-12-14 12:23, Henri Verbeet wrote: >> On 13 December 2016 at 19:48, Lauri Kenttä <lauri.kentta(a)gmail.com> >> wrote: >>> @@ -2027,6 +2027,19 @@ static int wctoint(WCHAR c, int base) >>> v = c - 'A' + 10; >>> else if ('a' <= c && c <= 'z') >>> v = c - 'a' + 10; >>> + else { >>> + /* Unicode points that contain digits 0-9; keep this >>> sorted! */ >>> + static const WCHAR zeros[] = { >>> + 0x660, 0x6f0, 0x966, 0x9e6, 0xa66, 0xae6, 0xb66, 0xc66, >>> 0xce6, >>> + 0xd66, 0xe50, 0xed0, 0x1040, 0x17e0, 0x1810, 0xff10 >>> + }; >>> + int i; >>> + for (i = 0; i < sizeof(zeros)/sizeof(zeros[0]) && c >= >>> zeros[i]; ++i) { >>> + if (zeros[i] <= c && c < zeros[i] + base) { >> Using "base" here seems questionable. That would imply that e.g. with >> base 16, "\x6f2\x6fa"/"۲ۺ" would return "42". Is that really the case? > > My bad, I've "optimized out" the missing c < zeros[i] + 10. > >> For what it's worth, note also that Wine has wine_fold_string(), which >> should be consistent with FoldString(). > > I don't see what FoldString could do here. MAP_FOLDDIGITS looks relevant to what you're doing.

Dec. 14, 2016

Re: [PATCH 3/3] msvcrt: Make wcstoi64 support various Unicode digits.

by Lauri Kenttä

On 2016-12-14 12:23, Henri Verbeet wrote: > On 13 December 2016 at 19:48, Lauri Kenttä <lauri.kentta(a)gmail.com> > wrote: >> @@ -2027,6 +2027,19 @@ static int wctoint(WCHAR c, int base) >> v = c - 'A' + 10; >> else if ('a' <= c && c <= 'z') >> v = c - 'a' + 10; >> + else { >> + /* Unicode points that contain digits 0-9; keep this sorted! >> */ >> + static const WCHAR zeros[] = { >> + 0x660, 0x6f0, 0x966, 0x9e6, 0xa66, 0xae6, 0xb66, 0xc66, >> 0xce6, >> + 0xd66, 0xe50, 0xed0, 0x1040, 0x17e0, 0x1810, 0xff10 >> + }; >> + int i; >> + for (i = 0; i < sizeof(zeros)/sizeof(zeros[0]) && c >= >> zeros[i]; ++i) { >> + if (zeros[i] <= c && c < zeros[i] + base) { > Using "base" here seems questionable. That would imply that e.g. with > base 16, "\x6f2\x6fa"/"۲ۺ" would return "42". Is that really the case? My bad, I've "optimized out" the missing c < zeros[i] + 10. > For what it's worth, note also that Wine has wine_fold_string(), which > should be consistent with FoldString(). I don't see what FoldString could do here. -- Lauri Kenttä

Dec. 14, 2016

Re: [PATCH 1/3] msvcrt/tests: Test wcstoi64 with various Unicode digits.

by Lauri Kenttä

On 2016-12-14 12:16, Piotr Caban wrote: > Hi Lauri, > > On 12/13/16 19:48, Lauri Kenttä wrote: >> + static const WCHAR zeros[] = { >> + 0xe50, 0xed0, 0x1040, 0x17e0, 0x1810, 0x660, 0x6f0, 0xff10, >> + 0x966, 0x9e6, 0xa66, 0xae6, 0xb66, 0xc66, 0xce6, 0xd66 >> + }; > Please also add a test for Tibetan digits range: > https://unicode-table.com/en/0F20/ > >> + /* Test various unicode digits */ >> + for (i = 0; i < sizeof(zeros) / sizeof(zeros[0]); ++i) { >> + WCHAR tmp[] = {zeros[i]+4, zeros[i], zeros[i]+5, 0}; >> + res = p_wcstoi64(tmp, NULL, 0); >> + todo_wine ok(res == 405, "with zero = %#x: got %d, expected >> 405\n", zeros[i], (int)res); > Could you please also add following test: > tmp[1] = zeros[i]+10; > res = p_wcstoi64(tmp, NULL, 16); > ok(res == 4, ...); > > Thank you, > Piotr Will do. -- Lauri Kenttä

Dec. 14, 2016

Re: Wined3d textureless rendering

by Henri Verbeet

On 14 December 2016 at 14:31, Bruno Jesus <00cpxxx(a)gmail.com> wrote: > Hello everyone. Following my email last year email asking if rendering > in wireframe could speedup a game (answer was no in many different > ways) I have a new idea after playing Totaly Accurate Battle Simulator > with its red and blue army. > > What if I skip applying every texture to every object? Probably > everything would be black so I would need to use different incremental > solid color to distinguish stuff in the screen. Would that save memory > and also make games faster? > Probably, in some cases, depending on where the performance bottleneck actually is. In fact, that's a performance analysis technique you can use to figure out if your (shader) performance is limited by texture lookups or by something else. That doesn't mean you can generally apply that to make things go faster though. Textures are used in a lot of places, sometimes unexpected ones. Think for example about text in the user interface, HUD elements, etc. If you want to experiment a bit, fixed function texture lookups happen in shader_glsl_generate_ffp_fragment_shader(), for shaders you'll want to look at anything that calls shader_glsl_get_sample_function(), perhaps most notably shader_glsl_tex().

Dec. 14, 2016

Wined3d textureless rendering

by Bruno Jesus

Hello everyone. Following my email last year email asking if rendering in wireframe could speedup a game (answer was no in many different ways) I have a new idea after playing Totaly Accurate Battle Simulator with its red and blue army. What if I skip applying every texture to every object? Probably everything would be black so I would need to use different incremental solid color to distinguish stuff in the screen. Would that save memory and also make games faster? Remembering that my 3D knowledge didn't change much so, basically zero + wireframe knowledge from previous email ;-) Best wishes, Bruno

Dec. 14, 2016