Messages in this thread | | | Subject | Re: [PATCH] x86/umip: Add emulation/spoofing for SLDT and STR instructions | From | Brendan Shanks <> | Date | Mon, 8 Jun 2020 11:13:44 -0700 |
| |
> On Jun 5, 2020, at 3:42 PM, Ricardo Neri <ricardo.neri-calderon@linux.intel.com> wrote: > > On Fri, Jun 05, 2020 at 11:58:13AM -0700, Brendan Shanks wrote: >> >>> On Jun 3, 2020, at 9:39 PM, Andy Lutomirski <luto@amacapital.net> wrote: >>> >>> On Wed, Jun 3, 2020 at 5:12 PM Ricardo Neri >>> <ricardo.neri-calderon@linux.intel.com <mailto:ricardo.neri-calderon@linux.intel.com>> 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 <andi@notmuch.email> >>>>> Originally-by: Ricardo Neri <ricardo.neri-calderon@linux.intel.com> >>>>> Signed-off-by: Brendan Shanks <bshanks@codeweavers.com> >>>>> --- >>>>> 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
| |