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 <[email protected]>
Originally-by: Ricardo Neri <[email protected]>
Signed-off-by: Brendan Shanks <[email protected]>
---
arch/x86/kernel/umip.c | 23 ++++++++++++++---------
1 file changed, 14 insertions(+), 9 deletions(-)
diff --git a/arch/x86/kernel/umip.c b/arch/x86/kernel/umip.c
index 8d5cbe1bbb3b..59dfceac5cc0 100644
--- a/arch/x86/kernel/umip.c
+++ b/arch/x86/kernel/umip.c
@@ -64,6 +64,8 @@
#define UMIP_DUMMY_GDT_BASE 0xfffffffffffe0000ULL
#define UMIP_DUMMY_IDT_BASE 0xffffffffffff0000ULL
+#define UMIP_DUMMY_TASK_REGISTER_SELECTOR 0x40
+
/*
* The SGDT and SIDT instructions store the contents of the global descriptor
* table and interrupt table registers, respectively. The destination is a
@@ -244,16 +246,24 @@ 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 = UMIP_DUMMY_TASK_REGISTER_SELECTOR;
+ else
+ dummy_value = 0;
/*
- * 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 +271,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 +392,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
On Tue, Jun 02, 2020 at 11:42:12AM -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 <[email protected]>
> Originally-by: Ricardo Neri <[email protected]>
> Signed-off-by: Brendan Shanks <[email protected]>
> ---
> arch/x86/kernel/umip.c | 23 ++++++++++++++---------
> 1 file changed, 14 insertions(+), 9 deletions(-)
>
> diff --git a/arch/x86/kernel/umip.c b/arch/x86/kernel/umip.c
> index 8d5cbe1bbb3b..59dfceac5cc0 100644
> --- a/arch/x86/kernel/umip.c
> +++ b/arch/x86/kernel/umip.c
> @@ -64,6 +64,8 @@
> #define UMIP_DUMMY_GDT_BASE 0xfffffffffffe0000ULL
> #define UMIP_DUMMY_IDT_BASE 0xffffffffffff0000ULL
>
> +#define UMIP_DUMMY_TASK_REGISTER_SELECTOR 0x40
> +
> /*
> * The SGDT and SIDT instructions store the contents of the global descriptor
> * table and interrupt table registers, respectively. The destination is a
> @@ -244,16 +246,24 @@ 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 = UMIP_DUMMY_TASK_REGISTER_SELECTOR;
> + else
> + dummy_value = 0;
Perhaps you can return a non-zero value for SLDT if it has an LDT, as
Andy had suggested. Maybe this can be implemented by looking at
current->mm->context.ldt
I guess the non-zero value can be (GDT_ENTRY_LDT*8).
Thanks and BR,
Ricardo
On Wed, Jun 3, 2020 at 5:12 PM Ricardo Neri
<[email protected]> wrote:
>
> On Tue, Jun 02, 2020 at 11:42:12AM -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 <[email protected]>
> > Originally-by: Ricardo Neri <[email protected]>
> > Signed-off-by: Brendan Shanks <[email protected]>
> > ---
> > arch/x86/kernel/umip.c | 23 ++++++++++++++---------
> > 1 file changed, 14 insertions(+), 9 deletions(-)
> >
> > diff --git a/arch/x86/kernel/umip.c b/arch/x86/kernel/umip.c
> > index 8d5cbe1bbb3b..59dfceac5cc0 100644
> > --- a/arch/x86/kernel/umip.c
> > +++ b/arch/x86/kernel/umip.c
> > @@ -64,6 +64,8 @@
> > #define UMIP_DUMMY_GDT_BASE 0xfffffffffffe0000ULL
> > #define UMIP_DUMMY_IDT_BASE 0xffffffffffff0000ULL
> >
> > +#define UMIP_DUMMY_TASK_REGISTER_SELECTOR 0x40
> > +
> > /*
> > * The SGDT and SIDT instructions store the contents of the global descriptor
> > * table and interrupt table registers, respectively. The destination is a
> > @@ -244,16 +246,24 @@ 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 = UMIP_DUMMY_TASK_REGISTER_SELECTOR;
> > + else
> > + dummy_value = 0;
>
> Perhaps you can return a non-zero value for SLDT if it has an LDT, as
> Andy had suggested. Maybe this can be implemented by looking at
> current->mm->context.ldt
>
> I guess the non-zero value can be (GDT_ENTRY_LDT*8).
You could probably even get away with always returning a nonzero
value. After all, an empty LDT is quite similar to no LDT.
--Andy
On Fri, Jun 05, 2020 at 11:58:13AM -0700, Brendan Shanks wrote:
>
> > On Jun 3, 2020, at 9:39 PM, Andy Lutomirski <[email protected]> wrote:
> >
> > On Wed, Jun 3, 2020 at 5:12 PM Ricardo Neri
> > <[email protected] <mailto:[email protected]>> wrote:
> >>
> >> On Tue, Jun 02, 2020 at 11:42:12AM -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 <[email protected]>
> >>> Originally-by: Ricardo Neri <[email protected]>
> >>> Signed-off-by: Brendan Shanks <[email protected]>
> >>> ---
> >>> arch/x86/kernel/umip.c | 23 ++++++++++++++---------
> >>> 1 file changed, 14 insertions(+), 9 deletions(-)
> >>>
> >>> diff --git a/arch/x86/kernel/umip.c b/arch/x86/kernel/umip.c
> >>> index 8d5cbe1bbb3b..59dfceac5cc0 100644
> >>> --- a/arch/x86/kernel/umip.c
> >>> +++ b/arch/x86/kernel/umip.c
> >>> @@ -64,6 +64,8 @@
> >>> #define UMIP_DUMMY_GDT_BASE 0xfffffffffffe0000ULL
> >>> #define UMIP_DUMMY_IDT_BASE 0xffffffffffff0000ULL
> >>>
> >>> +#define UMIP_DUMMY_TASK_REGISTER_SELECTOR 0x40
> >>> +
> >>> /*
> >>> * The SGDT and SIDT instructions store the contents of the global descriptor
> >>> * table and interrupt table registers, respectively. The destination is a
> >>> @@ -244,16 +246,24 @@ 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 = UMIP_DUMMY_TASK_REGISTER_SELECTOR;
> >>> + else
> >>> + dummy_value = 0;
> >>
> >> Perhaps you can return a non-zero value for SLDT if it has an LDT, as
> >> Andy had suggested. Maybe this can be implemented by looking at
> >> current->mm->context.ldt
> >>
> >> I guess the non-zero value can be (GDT_ENTRY_LDT*8).
> >
> > You could probably even get away with always returning a nonzero
> > value. After all, an empty LDT is quite similar to no LDT.
>
>
> Is something like this what you both had in mind?
> I don’t have any software handy to test the LDT-present case though.
Perhaps you can insert a test in the kernel selftest. Something like
this (based on Andreas' test program):
--- a/tools/testing/selftests/x86/ldt_gdt.c
+++ b/tools/testing/selftests/x86/ldt_gdt.c
@@ -220,12 +220,23 @@ static void install_invalid(const struct user_desc *desc, bool oldmode)
}
}
+unsigned long test(void)
+{
+ unsigned char ldtr[5] = "\xef\xbe\xad\xde";
+ unsigned long ldt = 0;
+ asm("sldt %0\n" : "=m" (ldtr));
+ ldt = *((unsigned long *)&ldtr[0]);
+ printf ("LDT base: 0x%lx\n", ldt);
+ return (ldt);
+}
+
static int safe_modify_ldt(int func, struct user_desc *ptr,
unsigned long bytecount)
{
int ret = syscall(SYS_modify_ldt, 0x11, ptr, bytecount);
if (ret < -1)
errno = -ret;
+ test();
return ret;
}
Thanks and BR,
Ricardo
>
>
> else if (umip_inst == UMIP_INST_STR)
> dummy_value = UMIP_DUMMY_TASK_REGISTER_SELECTOR;
> else if (umip_inst == UMIP_INST_SLDT)
> {
> #ifdef CONFIG_MODIFY_LDT_SYSCALL
> down_read(¤t->mm->context.ldt_usr_sem);
> if (current->mm->context.ldt)
> dummy_value = GDT_ENTRY_LDT * 8;
> else
> dummy_value = 0;
> up_read(¤t->mm->context.ldt_usr_sem);
> #else
> dummy_value = 0;
> #endif
>
It looks fine to me. Perhaps Andy prefers a simpler, always-non-zero
implementation?
Thanks and BR,
Ricardo
> On Jun 5, 2020, at 3:42 PM, Ricardo Neri <[email protected]> wrote:
>
> On Fri, Jun 05, 2020 at 11:58:13AM -0700, Brendan Shanks wrote:
>>
>>> On Jun 3, 2020, at 9:39 PM, Andy Lutomirski <[email protected]> wrote:
>>>
>>> On Wed, Jun 3, 2020 at 5:12 PM Ricardo Neri
>>> <[email protected] <mailto:[email protected]>> wrote:
>>>>
>>>> On Tue, Jun 02, 2020 at 11:42:12AM -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 <[email protected]>
>>>>> Originally-by: Ricardo Neri <[email protected]>
>>>>> Signed-off-by: Brendan Shanks <[email protected]>
>>>>> ---
>>>>> arch/x86/kernel/umip.c | 23 ++++++++++++++---------
>>>>> 1 file changed, 14 insertions(+), 9 deletions(-)
>>>>>
>>>>> diff --git a/arch/x86/kernel/umip.c b/arch/x86/kernel/umip.c
>>>>> index 8d5cbe1bbb3b..59dfceac5cc0 100644
>>>>> --- a/arch/x86/kernel/umip.c
>>>>> +++ b/arch/x86/kernel/umip.c
>>>>> @@ -64,6 +64,8 @@
>>>>> #define UMIP_DUMMY_GDT_BASE 0xfffffffffffe0000ULL
>>>>> #define UMIP_DUMMY_IDT_BASE 0xffffffffffff0000ULL
>>>>>
>>>>> +#define UMIP_DUMMY_TASK_REGISTER_SELECTOR 0x40
>>>>> +
>>>>> /*
>>>>> * The SGDT and SIDT instructions store the contents of the global descriptor
>>>>> * table and interrupt table registers, respectively. The destination is a
>>>>> @@ -244,16 +246,24 @@ 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 = UMIP_DUMMY_TASK_REGISTER_SELECTOR;
>>>>> + else
>>>>> + dummy_value = 0;
>>>>
>>>> Perhaps you can return a non-zero value for SLDT if it has an LDT, as
>>>> Andy had suggested. Maybe this can be implemented by looking at
>>>> current->mm->context.ldt
>>>>
>>>> I guess the non-zero value can be (GDT_ENTRY_LDT*8).
>>>
>>> You could probably even get away with always returning a nonzero
>>> value. After all, an empty LDT is quite similar to no LDT.
>>
>>
>> Is something like this what you both had in mind?
>
>> I don’t have any software handy to test the LDT-present case though.
>
> Perhaps you can insert a test in the kernel selftest. Something like
> this (based on Andreas' test program):
>
> --- a/tools/testing/selftests/x86/ldt_gdt.c
> +++ b/tools/testing/selftests/x86/ldt_gdt.c
> @@ -220,12 +220,23 @@ static void install_invalid(const struct user_desc *desc, bool oldmode)
> }
> }
>
> +unsigned long test(void)
> +{
> + unsigned char ldtr[5] = "\xef\xbe\xad\xde";
> + unsigned long ldt = 0;
> + asm("sldt %0\n" : "=m" (ldtr));
> + ldt = *((unsigned long *)&ldtr[0]);
> + printf ("LDT base: 0x%lx\n", ldt);
> + return (ldt);
> +}
> +
> static int safe_modify_ldt(int func, struct user_desc *ptr,
> unsigned long bytecount)
> {
> int ret = syscall(SYS_modify_ldt, 0x11, ptr, bytecount);
> if (ret < -1)
> errno = -ret;
> + test();
> return ret;
> }
Thanks for the tip, I gave that a try and got the same results under UMIP and a non-UMIP system. I’ll send this next version of the patch out.
Brendan Shanks
CodeWeavers