All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v5] x86/umip: Add emulation/spoofing for SLDT and STR instructions
@ 2020-07-10 22:45 Brendan Shanks
  2020-07-11 17:13 ` Andy Lutomirski
                   ` (3 more replies)
  0 siblings, 4 replies; 7+ messages in thread
From: Brendan Shanks @ 2020-07-10 22:45 UTC (permalink / raw)
  To: linux-kernel
  Cc: ricardo.neri-calderon, tglx, mingo, bp, hpa, x86, ebiederm, andi,
	Babu.Moger, 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>
---

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, &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


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* Re: [PATCH v5] x86/umip: Add emulation/spoofing for SLDT and STR instructions
  2020-07-10 22:45 [PATCH v5] x86/umip: Add emulation/spoofing for SLDT and STR instructions Brendan Shanks
@ 2020-07-11 17:13 ` Andy Lutomirski
  2020-07-11 21:49 ` hpa
                   ` (2 subsequent siblings)
  3 siblings, 0 replies; 7+ messages in thread
From: Andy Lutomirski @ 2020-07-11 17:13 UTC (permalink / raw)
  To: Brendan Shanks
  Cc: LKML, Ricardo Neri, Thomas Gleixner, Ingo Molnar,
	Borislav Petkov, H. Peter Anvin, X86 ML, Eric W. Biederman,
	Andreas Rammhold, Moger, Babu

On Fri, Jul 10, 2020 at 3:45 PM Brendan Shanks <bshanks@codeweavers.com> 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 <luto@kernel.org>

I tested this under KVM-emulated UMIP.  I don't have a real UMIP
system handle to test STR.

--Andy

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH v5] x86/umip: Add emulation/spoofing for SLDT and STR instructions
  2020-07-10 22:45 [PATCH v5] x86/umip: Add emulation/spoofing for SLDT and STR instructions Brendan Shanks
  2020-07-11 17:13 ` Andy Lutomirski
@ 2020-07-11 21:49 ` hpa
  2020-07-13 23:45   ` Ricardo Neri
  2020-07-13 23:59 ` Ricardo Neri
  2020-08-20 17:27 ` [tip: x86/cpu] " tip-bot2 for Brendan Shanks
  3 siblings, 1 reply; 7+ messages in thread
From: hpa @ 2020-07-11 21:49 UTC (permalink / raw)
  To: Brendan Shanks, linux-kernel
  Cc: ricardo.neri-calderon, tglx, mingo, bp, x86, ebiederm, andi, Babu.Moger

On July 10, 2020 3:45:25 PM PDT, Brendan Shanks <bshanks@codeweavers.com> 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>
>---
>
>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, &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,

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.

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH v5] x86/umip: Add emulation/spoofing for SLDT and STR instructions
  2020-07-11 21:49 ` hpa
@ 2020-07-13 23:45   ` Ricardo Neri
  2020-07-14  1:10     ` Andy Lutomirski
  0 siblings, 1 reply; 7+ messages in thread
From: Ricardo Neri @ 2020-07-13 23:45 UTC (permalink / raw)
  To: hpa
  Cc: Brendan Shanks, linux-kernel, tglx, mingo, bp, x86, ebiederm,
	andi, Babu.Moger

On Sat, Jul 11, 2020 at 02:49:54PM -0700, hpa@zytor.com wrote:
> On July 10, 2020 3:45:25 PM PDT, Brendan Shanks <bshanks@codeweavers.com> 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>
> >---
> >
> >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, &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,
> 
> 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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH v5] x86/umip: Add emulation/spoofing for SLDT and STR instructions
  2020-07-10 22:45 [PATCH v5] x86/umip: Add emulation/spoofing for SLDT and STR instructions Brendan Shanks
  2020-07-11 17:13 ` Andy Lutomirski
  2020-07-11 21:49 ` hpa
@ 2020-07-13 23:59 ` Ricardo Neri
  2020-08-20 17:27 ` [tip: x86/cpu] " tip-bot2 for Brendan Shanks
  3 siblings, 0 replies; 7+ messages in thread
From: Ricardo Neri @ 2020-07-13 23:59 UTC (permalink / raw)
  To: Brendan Shanks
  Cc: linux-kernel, tglx, mingo, bp, hpa, x86, ebiederm, andi, Babu.Moger

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

FWIW, tested on hardware with UMIP.

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

Thanks and BR,
Ricardo

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH v5] x86/umip: Add emulation/spoofing for SLDT and STR instructions
  2020-07-13 23:45   ` Ricardo Neri
@ 2020-07-14  1:10     ` Andy Lutomirski
  0 siblings, 0 replies; 7+ messages in thread
From: Andy Lutomirski @ 2020-07-14  1:10 UTC (permalink / raw)
  To: Ricardo Neri
  Cc: H. Peter Anvin, Brendan Shanks, LKML, Thomas Gleixner,
	Ingo Molnar, Borislav Petkov, X86 ML, Eric W. Biederman,
	Andreas Rammhold, Moger, Babu

On Mon, Jul 13, 2020 at 4:45 PM Ricardo Neri
<ricardo.neri-calderon@linux.intel.com> wrote:
>
> On Sat, Jul 11, 2020 at 02:49:54PM -0700, hpa@zytor.com wrote:
> > On July 10, 2020 3:45:25 PM PDT, Brendan Shanks <bshanks@codeweavers.com> 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>
> > >---
> > >
> > >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, &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,
> >
> > 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.

You're understanding the LDT right, but that's none of the user's
business.  hpa may well be right -- unless it actively causes
problems, we might as well return a constant value.

^ permalink raw reply	[flat|nested] 7+ messages in thread

* [tip: x86/cpu] x86/umip: Add emulation/spoofing for SLDT and STR instructions
  2020-07-10 22:45 [PATCH v5] x86/umip: Add emulation/spoofing for SLDT and STR instructions Brendan Shanks
                   ` (2 preceding siblings ...)
  2020-07-13 23:59 ` Ricardo Neri
@ 2020-08-20 17:27 ` tip-bot2 for Brendan Shanks
  3 siblings, 0 replies; 7+ messages in thread
From: tip-bot2 for Brendan Shanks @ 2020-08-20 17:27 UTC (permalink / raw)
  To: linux-tip-commits
  Cc: Ricardo Neri, Andreas Rammhold, Brendan Shanks, Borislav Petkov,
	Andy Lutomirski, x86, LKML

The following commit has been merged into the x86/cpu branch of tip:

Commit-ID:     b91e7089ae70d2f7c81a4456e5b78fef498663d9
Gitweb:        https://git.kernel.org/tip/b91e7089ae70d2f7c81a4456e5b78fef498663d9
Author:        Brendan Shanks <bshanks@codeweavers.com>
AuthorDate:    Fri, 10 Jul 2020 15:45:25 -07:00
Committer:     Borislav Petkov <bp@suse.de>
CommitterDate: Thu, 20 Aug 2020 19:10:26 +02:00

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.

Originally-by: Ricardo Neri <ricardo.neri-calderon@linux.intel.com>
Reported-by: Andreas Rammhold <andi@notmuch.email>
Signed-off-by: Brendan Shanks <bshanks@codeweavers.com>
Signed-off-by: Borislav Petkov <bp@suse.de>
Acked-by: Andy Lutomirski <luto@kernel.org>
Reviewed-by: Ricardo Neri <ricardo.neri-calderon@linux.intel.com>
Tested-by: Ricardo Neri <ricardo.neri-calderon@linux.intel.com>
Link: https://lkml.kernel.org/r/20200710224525.21966-1-bshanks@codeweavers.com
---
 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 8d5cbe1..2c304fd 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, &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,

^ permalink raw reply related	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2020-08-20 17:27 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-07-10 22:45 [PATCH v5] x86/umip: Add emulation/spoofing for SLDT and STR instructions Brendan Shanks
2020-07-11 17:13 ` Andy Lutomirski
2020-07-11 21:49 ` hpa
2020-07-13 23:45   ` Ricardo Neri
2020-07-14  1:10     ` Andy Lutomirski
2020-07-13 23:59 ` Ricardo Neri
2020-08-20 17:27 ` [tip: x86/cpu] " tip-bot2 for Brendan Shanks

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.