[PATCH v4] x86/umip: Add emulation/spoofing for SLDT and STR instructions

[PATCH v4] x86/umip: Add emulation/spoofing for SLDT and STR instructions

[Brendan Shanks]

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 <andi@notmuch.email>
Originally-by: Ricardo Neri <ricardo.neri-calderon@linux.intel.com>
Signed-off-by: Brendan Shanks <bshanks@codeweavers.com>
---

v4: Use braces for every clause of the conditional. I tried a switch(),
but it takes more lines and looks more cluttered (especially with the
#ifdef).
Also replace out-of-date comment at top of file.

 arch/x86/kernel/umip.c | 38 ++++++++++++++++++++++++++------------
 1 file changed, 26 insertions(+), 12 deletions(-)

diff --git a/arch/x86/kernel/umip.c b/arch/x86/kernel/umip.c
index 8d5cbe1bbb3b..62f4f0afb979 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
  * 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, &dummy_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(&current->mm->context.ldt_usr_sem);
+			if (current->mm->context.ldt)
+				dummy_value = GDT_ENTRY_LDT * 8;
+			else
+				dummy_value = 0;
+			up_read(&current->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, &dummy_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(&insn, umip_inst, dummy_data, &dummy_data_size,
-- 
2.26.2

Re: [PATCH v4] x86/umip: Add emulation/spoofing for SLDT and STR instructions

[Ricardo Neri]

On Tue, Jun 09, 2020 at 10:54:23AM -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 <andi@notmuch.email>
> Originally-by: Ricardo Neri <ricardo.neri-calderon@linux.intel.com>
> Signed-off-by: Brendan Shanks <bshanks@codeweavers.com>
> ---
> 
> v4: Use braces for every clause of the conditional. I tried a switch(),
> but it takes more lines and looks more cluttered (especially with the
> #ifdef).
> Also replace out-of-date comment at top of file.
> 
>  arch/x86/kernel/umip.c | 38 ++++++++++++++++++++++++++------------
>  1 file changed, 26 insertions(+), 12 deletions(-)
> 
> diff --git a/arch/x86/kernel/umip.c b/arch/x86/kernel/umip.c
> index 8d5cbe1bbb3b..62f4f0afb979 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
>   * 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).

Probably for consistency with the rest of the document the instruction
names should be capitalized (I know that smsw was not capitalized before
:/). Maybe maintainers can fix this when applying?

Other than that, FWIW:

Reviewed-by: Ricardo Neri <ricardo.neri-calderon@linux.intel.com>
Tested-by: Ricardo Neri <ricardo.neri-calderon@linux.intel.com>

Re: [PATCH v4] x86/umip: Add emulation/spoofing for SLDT and STR instructions

[andi]

On 10:54 09.06.20, 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 <andi@notmuch.email>
> Originally-by: Ricardo Neri <ricardo.neri-calderon@linux.intel.com>
> Signed-off-by: Brendan Shanks <bshanks@codeweavers.com>
> ---
> 
> v4: Use braces for every clause of the conditional. I tried a switch(),
> but it takes more lines and looks more cluttered (especially with the
> #ifdef).
> Also replace out-of-date comment at top of file.
> 
>  arch/x86/kernel/umip.c | 38 ++++++++++++++++++++++++++------------
>  1 file changed, 26 insertions(+), 12 deletions(-)
> 
> diff --git a/arch/x86/kernel/umip.c b/arch/x86/kernel/umip.c
> index 8d5cbe1bbb3b..62f4f0afb979 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
>   * 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, &dummy_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(&current->mm->context.ldt_usr_sem);
> +			if (current->mm->context.ldt)
> +				dummy_value = GDT_ENTRY_LDT * 8;
> +			else
> +				dummy_value = 0;
> +			up_read(&current->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, &dummy_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(&insn, umip_inst, dummy_data, &dummy_data_size,
> -- 
> 2.26.2
> 

A bit late but I was able to test my workload with the above patch
applied on 5.7.2.

Tested-by: Andreas Rammhold <andi@notmuch.email>