Re: [PATCH v5] x86/umip: Add emulation/spoofing for SLDT and STR instructions
On Mon, Jul 13, 2020 at 4:45 PM Ricardo Neri wrote: > > On Sat, Jul 11, 2020 at 02:49:54PM -0700, h...@zytor.com wrote: > > On July 10, 2020 3:45:25 PM PDT, Brendan Shanks > > wrote: > > >Add emulation/spoofing of SLDT and STR for both 32- and 64-bit > > >processes. > > > > > >Wine users have found a small number of Windows apps using SLDT that > > >were crashing when run on UMIP-enabled systems. > > > > > >Reported-by: Andreas Rammhold > > >Originally-by: Ricardo Neri > > >Signed-off-by: Brendan Shanks > > >--- > > > > > >v5: Capitalize instruction names in comments. > > > > > > arch/x86/kernel/umip.c | 40 +++- > > > 1 file changed, 27 insertions(+), 13 deletions(-) > > > > > >diff --git a/arch/x86/kernel/umip.c b/arch/x86/kernel/umip.c > > >index 8d5cbe1bbb3b..2c304fd0bb1a 100644 > > >--- a/arch/x86/kernel/umip.c > > >+++ b/arch/x86/kernel/umip.c > > >@@ -45,11 +45,12 @@ > > >* value that, lies close to the top of the kernel memory. The limit for > > >the GDT > > > * and the IDT are set to zero. > > > * > > >- * Given that SLDT and STR are not commonly used in programs that run > > >on WineHQ > > >- * or DOSEMU2, they are not emulated. > > >- * > > >- * The instruction smsw is emulated to return the value that the > > >register CR0 > > >+ * The instruction SMSW is emulated to return the value that the > > >register CR0 > > > * has at boot time as set in the head_32. > > >+ * SLDT and STR are emulated to return the values that the kernel > > >programmatically > > >+ * assigns: > > >+ * - SLDT returns (GDT_ENTRY_LDT * 8) if an LDT has been set, 0 if > > >not. > > >+ * - STR returns (GDT_ENTRY_TSS * 8). > > > * > > > * Emulation is provided for both 32-bit and 64-bit processes. > > > * > > >@@ -244,16 +245,34 @@ static int emulate_umip_insn(struct insn *insn, > > >int umip_inst, > > > *data_size += UMIP_GDT_IDT_LIMIT_SIZE; > > > memcpy(data, _limit, UMIP_GDT_IDT_LIMIT_SIZE); > > > > > >-} else if (umip_inst == UMIP_INST_SMSW) { > > >-unsigned long dummy_value = CR0_STATE; > > >+} else if (umip_inst == UMIP_INST_SMSW || umip_inst == UMIP_INST_SLDT > > >|| > > >+ umip_inst == UMIP_INST_STR) { > > >+unsigned long dummy_value; > > >+ > > >+if (umip_inst == UMIP_INST_SMSW) { > > >+dummy_value = CR0_STATE; > > >+} else if (umip_inst == UMIP_INST_STR) { > > >+dummy_value = GDT_ENTRY_TSS * 8; > > >+} else if (umip_inst == UMIP_INST_SLDT) { > > >+#ifdef CONFIG_MODIFY_LDT_SYSCALL > > >+down_read(>mm->context.ldt_usr_sem); > > >+if (current->mm->context.ldt) > > >+dummy_value = GDT_ENTRY_LDT * 8; > > >+else > > >+dummy_value = 0; > > >+up_read(>mm->context.ldt_usr_sem); > > >+#else > > >+dummy_value = 0; > > >+#endif > > >+} > > > > > > /* > > >- * Even though the CR0 register has 4 bytes, the number > > >+ * For these 3 instructions, the number > > > * of bytes to be copied in the result buffer is determined > > > * by whether the operand is a register or a memory location. > > > * If operand is a register, return as many bytes as the > > > operand > > > * size. If operand is memory, return only the two least > > >- * siginificant bytes of CR0. > > >+ * siginificant bytes. > > > */ > > > if (X86_MODRM_MOD(insn->modrm.value) == 3) > > > *data_size = insn->opnd_bytes; > > >@@ -261,7 +280,6 @@ static int emulate_umip_insn(struct insn *insn, int > > >umip_inst, > > > *data_size = 2; > > > > > > memcpy(data, _value, *data_size); > > >-/* STR and SLDT are not emulated */ > > > } else { > > > return -EINVAL; > > > } > > >@@ -383,10 +401,6 @@ bool fixup_umip_exception(struct pt_regs *regs) > > > umip_pr_warn(regs, "%s instruction cannot be used by applications.\n", > > > umip_insns[umip_inst]); > > > > > >-/* Do not emulate (spoof) SLDT or STR. */ > > >-if (umip_inst == UMIP_INST_STR || umip_inst == UMIP_INST_SLDT) > > >-return false; > > >- > > > umip_pr_warn(regs, "For now, expensive software emulation returns the > > >result.\n"); > > > > > > if (emulate_umip_insn(, umip_inst, dummy_data, _data_size, > > > > It's there any reason for SLDT to not *always* return a fixed value? "An > > LDT has been assigned" is formally a kernel internal property, separate > > from the property of whenever there are user space enteies in the LDT. > > But isn't it true that sldt returns 0 if the application has not set an > LDT and non-zero otherwise? > > In native_set_ldt() I see that the the LDT register is set to 0 if
Re: [PATCH v5] x86/umip: Add emulation/spoofing for SLDT and STR instructions
On Fri, Jul 10, 2020 at 03:45:25PM -0700, Brendan Shanks wrote: > Add emulation/spoofing of SLDT and STR for both 32- and 64-bit > processes. > > Wine users have found a small number of Windows apps using SLDT that > were crashing when run on UMIP-enabled systems. > > Reported-by: Andreas Rammhold > Originally-by: Ricardo Neri > Signed-off-by: Brendan Shanks FWIW, tested on hardware with UMIP. Reviewed-by: Ricardo Neri Tested-by: Ricardo Neri Thanks and BR, Ricardo
Re: [PATCH v5] x86/umip: Add emulation/spoofing for SLDT and STR instructions
On Sat, Jul 11, 2020 at 02:49:54PM -0700, h...@zytor.com wrote: > On July 10, 2020 3:45:25 PM PDT, Brendan Shanks > wrote: > >Add emulation/spoofing of SLDT and STR for both 32- and 64-bit > >processes. > > > >Wine users have found a small number of Windows apps using SLDT that > >were crashing when run on UMIP-enabled systems. > > > >Reported-by: Andreas Rammhold > >Originally-by: Ricardo Neri > >Signed-off-by: Brendan Shanks > >--- > > > >v5: Capitalize instruction names in comments. > > > > arch/x86/kernel/umip.c | 40 +++- > > 1 file changed, 27 insertions(+), 13 deletions(-) > > > >diff --git a/arch/x86/kernel/umip.c b/arch/x86/kernel/umip.c > >index 8d5cbe1bbb3b..2c304fd0bb1a 100644 > >--- a/arch/x86/kernel/umip.c > >+++ b/arch/x86/kernel/umip.c > >@@ -45,11 +45,12 @@ > >* value that, lies close to the top of the kernel memory. The limit for > >the GDT > > * and the IDT are set to zero. > > * > >- * Given that SLDT and STR are not commonly used in programs that run > >on WineHQ > >- * or DOSEMU2, they are not emulated. > >- * > >- * The instruction smsw is emulated to return the value that the > >register CR0 > >+ * The instruction SMSW is emulated to return the value that the > >register CR0 > > * has at boot time as set in the head_32. > >+ * SLDT and STR are emulated to return the values that the kernel > >programmatically > >+ * assigns: > >+ * - SLDT returns (GDT_ENTRY_LDT * 8) if an LDT has been set, 0 if > >not. > >+ * - STR returns (GDT_ENTRY_TSS * 8). > > * > > * Emulation is provided for both 32-bit and 64-bit processes. > > * > >@@ -244,16 +245,34 @@ static int emulate_umip_insn(struct insn *insn, > >int umip_inst, > > *data_size += UMIP_GDT_IDT_LIMIT_SIZE; > > memcpy(data, _limit, UMIP_GDT_IDT_LIMIT_SIZE); > > > >-} else if (umip_inst == UMIP_INST_SMSW) { > >-unsigned long dummy_value = CR0_STATE; > >+} else if (umip_inst == UMIP_INST_SMSW || umip_inst == UMIP_INST_SLDT > >|| > >+ umip_inst == UMIP_INST_STR) { > >+unsigned long dummy_value; > >+ > >+if (umip_inst == UMIP_INST_SMSW) { > >+dummy_value = CR0_STATE; > >+} else if (umip_inst == UMIP_INST_STR) { > >+dummy_value = GDT_ENTRY_TSS * 8; > >+} else if (umip_inst == UMIP_INST_SLDT) { > >+#ifdef CONFIG_MODIFY_LDT_SYSCALL > >+down_read(>mm->context.ldt_usr_sem); > >+if (current->mm->context.ldt) > >+dummy_value = GDT_ENTRY_LDT * 8; > >+else > >+dummy_value = 0; > >+up_read(>mm->context.ldt_usr_sem); > >+#else > >+dummy_value = 0; > >+#endif > >+} > > > > /* > >- * Even though the CR0 register has 4 bytes, the number > >+ * For these 3 instructions, the number > > * of bytes to be copied in the result buffer is determined > > * by whether the operand is a register or a memory location. > > * If operand is a register, return as many bytes as the operand > > * size. If operand is memory, return only the two least > >- * siginificant bytes of CR0. > >+ * siginificant bytes. > > */ > > if (X86_MODRM_MOD(insn->modrm.value) == 3) > > *data_size = insn->opnd_bytes; > >@@ -261,7 +280,6 @@ static int emulate_umip_insn(struct insn *insn, int > >umip_inst, > > *data_size = 2; > > > > memcpy(data, _value, *data_size); > >-/* STR and SLDT are not emulated */ > > } else { > > return -EINVAL; > > } > >@@ -383,10 +401,6 @@ bool fixup_umip_exception(struct pt_regs *regs) > > umip_pr_warn(regs, "%s instruction cannot be used by applications.\n", > > umip_insns[umip_inst]); > > > >-/* Do not emulate (spoof) SLDT or STR. */ > >-if (umip_inst == UMIP_INST_STR || umip_inst == UMIP_INST_SLDT) > >-return false; > >- > > umip_pr_warn(regs, "For now, expensive software emulation returns the > >result.\n"); > > > > if (emulate_umip_insn(, umip_inst, dummy_data, _data_size, > > It's there any reason for SLDT to not *always* return a fixed value? "An LDT > has been assigned" is formally a kernel internal property, separate from the > property of whenever there are user space enteies in the LDT. But isn't it true that sldt returns 0 if the application has not set an LDT and non-zero otherwise? In native_set_ldt() I see that the the LDT register is set to 0 if the table has no entries and to GDT_ENTRY_LDT*8 otherwise. Please correct me if I understand this wrong. Thanks and BR, Ricardo
Re: [PATCH v5] x86/umip: Add emulation/spoofing for SLDT and STR instructions
On July 10, 2020 3:45:25 PM PDT, Brendan Shanks wrote: >Add emulation/spoofing of SLDT and STR for both 32- and 64-bit >processes. > >Wine users have found a small number of Windows apps using SLDT that >were crashing when run on UMIP-enabled systems. > >Reported-by: Andreas Rammhold >Originally-by: Ricardo Neri >Signed-off-by: Brendan Shanks >--- > >v5: Capitalize instruction names in comments. > > arch/x86/kernel/umip.c | 40 +++- > 1 file changed, 27 insertions(+), 13 deletions(-) > >diff --git a/arch/x86/kernel/umip.c b/arch/x86/kernel/umip.c >index 8d5cbe1bbb3b..2c304fd0bb1a 100644 >--- a/arch/x86/kernel/umip.c >+++ b/arch/x86/kernel/umip.c >@@ -45,11 +45,12 @@ >* value that, lies close to the top of the kernel memory. The limit for >the GDT > * and the IDT are set to zero. > * >- * Given that SLDT and STR are not commonly used in programs that run >on WineHQ >- * or DOSEMU2, they are not emulated. >- * >- * The instruction smsw is emulated to return the value that the >register CR0 >+ * The instruction SMSW is emulated to return the value that the >register CR0 > * has at boot time as set in the head_32. >+ * SLDT and STR are emulated to return the values that the kernel >programmatically >+ * assigns: >+ * - SLDT returns (GDT_ENTRY_LDT * 8) if an LDT has been set, 0 if >not. >+ * - STR returns (GDT_ENTRY_TSS * 8). > * > * Emulation is provided for both 32-bit and 64-bit processes. > * >@@ -244,16 +245,34 @@ static int emulate_umip_insn(struct insn *insn, >int umip_inst, > *data_size += UMIP_GDT_IDT_LIMIT_SIZE; > memcpy(data, _limit, UMIP_GDT_IDT_LIMIT_SIZE); > >- } else if (umip_inst == UMIP_INST_SMSW) { >- unsigned long dummy_value = CR0_STATE; >+ } else if (umip_inst == UMIP_INST_SMSW || umip_inst == UMIP_INST_SLDT >|| >+ umip_inst == UMIP_INST_STR) { >+ unsigned long dummy_value; >+ >+ if (umip_inst == UMIP_INST_SMSW) { >+ dummy_value = CR0_STATE; >+ } else if (umip_inst == UMIP_INST_STR) { >+ dummy_value = GDT_ENTRY_TSS * 8; >+ } else if (umip_inst == UMIP_INST_SLDT) { >+#ifdef CONFIG_MODIFY_LDT_SYSCALL >+ down_read(>mm->context.ldt_usr_sem); >+ if (current->mm->context.ldt) >+ dummy_value = GDT_ENTRY_LDT * 8; >+ else >+ dummy_value = 0; >+ up_read(>mm->context.ldt_usr_sem); >+#else >+ dummy_value = 0; >+#endif >+ } > > /* >- * Even though the CR0 register has 4 bytes, the number >+ * For these 3 instructions, the number >* of bytes to be copied in the result buffer is determined >* by whether the operand is a register or a memory location. >* If operand is a register, return as many bytes as the operand >* size. If operand is memory, return only the two least >- * siginificant bytes of CR0. >+ * siginificant bytes. >*/ > if (X86_MODRM_MOD(insn->modrm.value) == 3) > *data_size = insn->opnd_bytes; >@@ -261,7 +280,6 @@ static int emulate_umip_insn(struct insn *insn, int >umip_inst, > *data_size = 2; > > memcpy(data, _value, *data_size); >- /* STR and SLDT are not emulated */ > } else { > return -EINVAL; > } >@@ -383,10 +401,6 @@ bool fixup_umip_exception(struct pt_regs *regs) > umip_pr_warn(regs, "%s instruction cannot be used by applications.\n", > umip_insns[umip_inst]); > >- /* Do not emulate (spoof) SLDT or STR. */ >- if (umip_inst == UMIP_INST_STR || umip_inst == UMIP_INST_SLDT) >- return false; >- > umip_pr_warn(regs, "For now, expensive software emulation returns the >result.\n"); > > if (emulate_umip_insn(, umip_inst, dummy_data, _data_size, It's there any reason for SLDT to not *always* return a fixed value? "An LDT has been assigned" is formally a kernel internal property, separate from the property of whenever there are user space enteies in the LDT. -- Sent from my Android device with K-9 Mail. Please excuse my brevity.
Re: [PATCH v5] x86/umip: Add emulation/spoofing for SLDT and STR instructions
On Fri, Jul 10, 2020 at 3:45 PM Brendan Shanks wrote: > > Add emulation/spoofing of SLDT and STR for both 32- and 64-bit > processes. > > Wine users have found a small number of Windows apps using SLDT that > were crashing when run on UMIP-enabled systems. > Acked-by: Andy Lutomirski I tested this under KVM-emulated UMIP. I don't have a real UMIP system handle to test STR. --Andy
[PATCH v5] x86/umip: Add emulation/spoofing for SLDT and STR instructions
Add emulation/spoofing of SLDT and STR for both 32- and 64-bit processes. Wine users have found a small number of Windows apps using SLDT that were crashing when run on UMIP-enabled systems. Reported-by: Andreas Rammhold Originally-by: Ricardo Neri Signed-off-by: Brendan Shanks --- v5: Capitalize instruction names in comments. arch/x86/kernel/umip.c | 40 +++- 1 file changed, 27 insertions(+), 13 deletions(-) diff --git a/arch/x86/kernel/umip.c b/arch/x86/kernel/umip.c index 8d5cbe1bbb3b..2c304fd0bb1a 100644 --- a/arch/x86/kernel/umip.c +++ b/arch/x86/kernel/umip.c @@ -45,11 +45,12 @@ * value that, lies close to the top of the kernel memory. The limit for the GDT * and the IDT are set to zero. * - * Given that SLDT and STR are not commonly used in programs that run on WineHQ - * or DOSEMU2, they are not emulated. - * - * The instruction smsw is emulated to return the value that the register CR0 + * The instruction SMSW is emulated to return the value that the register CR0 * has at boot time as set in the head_32. + * SLDT and STR are emulated to return the values that the kernel programmatically + * assigns: + * - SLDT returns (GDT_ENTRY_LDT * 8) if an LDT has been set, 0 if not. + * - STR returns (GDT_ENTRY_TSS * 8). * * Emulation is provided for both 32-bit and 64-bit processes. * @@ -244,16 +245,34 @@ static int emulate_umip_insn(struct insn *insn, int umip_inst, *data_size += UMIP_GDT_IDT_LIMIT_SIZE; memcpy(data, _limit, UMIP_GDT_IDT_LIMIT_SIZE); - } else if (umip_inst == UMIP_INST_SMSW) { - unsigned long dummy_value = CR0_STATE; + } else if (umip_inst == UMIP_INST_SMSW || umip_inst == UMIP_INST_SLDT || + umip_inst == UMIP_INST_STR) { + unsigned long dummy_value; + + if (umip_inst == UMIP_INST_SMSW) { + dummy_value = CR0_STATE; + } else if (umip_inst == UMIP_INST_STR) { + dummy_value = GDT_ENTRY_TSS * 8; + } else if (umip_inst == UMIP_INST_SLDT) { +#ifdef CONFIG_MODIFY_LDT_SYSCALL + down_read(>mm->context.ldt_usr_sem); + if (current->mm->context.ldt) + dummy_value = GDT_ENTRY_LDT * 8; + else + dummy_value = 0; + up_read(>mm->context.ldt_usr_sem); +#else + dummy_value = 0; +#endif + } /* -* Even though the CR0 register has 4 bytes, the number +* For these 3 instructions, the number * of bytes to be copied in the result buffer is determined * by whether the operand is a register or a memory location. * If operand is a register, return as many bytes as the operand * size. If operand is memory, return only the two least -* siginificant bytes of CR0. +* siginificant bytes. */ if (X86_MODRM_MOD(insn->modrm.value) == 3) *data_size = insn->opnd_bytes; @@ -261,7 +280,6 @@ static int emulate_umip_insn(struct insn *insn, int umip_inst, *data_size = 2; memcpy(data, _value, *data_size); - /* STR and SLDT are not emulated */ } else { return -EINVAL; } @@ -383,10 +401,6 @@ bool fixup_umip_exception(struct pt_regs *regs) umip_pr_warn(regs, "%s instruction cannot be used by applications.\n", umip_insns[umip_inst]); - /* Do not emulate (spoof) SLDT or STR. */ - if (umip_inst == UMIP_INST_STR || umip_inst == UMIP_INST_SLDT) - return false; - umip_pr_warn(regs, "For now, expensive software emulation returns the result.\n"); if (emulate_umip_insn(, umip_inst, dummy_data, _data_size, -- 2.26.2