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

[andi@notmuch.email]

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>