Re: [v2 5/7] x86: Add emulation code for UMIP instructions

[Ricardo Neri]

On Mon, 2016-12-26 at 00:49 +0900, Masami Hiramatsu wrote:
> On Fri, 23 Dec 2016 17:37:43 -0800
> Ricardo Neri  wrote:
> 
> > +static int __identify_insn(struct insn *insn)
> > +{
> > +   /* by getting modrm we also get the opcode */
> > +   insn_get_modrm(insn);
> > +   if (insn->opcode.bytes[0] != 0xf)
> > +   return -EINVAL;
> > +
> > +   if (insn->opcode.bytes[1] == 0x1) {
> > +   switch (X86_MODRM_REG(insn->modrm.value)) {
> > +   case 0:
> > +   return UMIP_SGDT;
> > +   case 1:
> > +   return UMIP_SIDT;
> > +   case 4:
> > +   return UMIP_SMSW;
> > +   default:
> > +   return -EINVAL;
> > +   }
> > +   } else if (insn->opcode.bytes[1] == 0x0) {
> > +   if (X86_MODRM_REG(insn->modrm.value) == 0)
> > +   return UMIP_SLDT;
> > +   else if (X86_MODRM_REG(insn->modrm.value) == 1)
> > +   return UMIP_STR;
> > +   else
> > +   return -EINVAL;
> > +   }
> 
> gcc detected an error here, you may need return "-EINVAL".

I will make this change. I removed this EINVAL at the last minute as it
didn't look right. It was indeed right.

Thanks and BR,
Ricardo
> 
> Thanks,
> 
> 
> 


--
To unsubscribe from this list: send the line "unsubscribe linux-msdos" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: lag while printing and serial redirect

[Jakub Klawiter]

Hello!

2016-12-25 17:58 GMT+01:00 Stas Sergeev :

>>> You mean -t.
>> I've been using -dt but ok … it looks the same so probably -d (in my
>> dosemu) does nothing ;-)
> I never told  you to use -d. I told either -t or -td, but
> not -d or -dt.
:D I just wanted to say that -d does nothing so -td works same as -t.
at the end -d -t == -t -d == -td ==-dt isn't it?

>> anyway same dos exe i'm using is working fine with dosemu started with
>> -X so maybe it is possible.
> Its certainly possible with -X, the problem exists only
> with -t. So after you outlined it, I withdrawn my suggestion
> about -t. -t is good to just run a bat file, which I thought
> your use-case is. For interactive stuff it doesn't work well.

ok, once again thank you! :)

and Happy New year All! :)

-- 
Pozdrawiam, Jakub.
Milion osób dziennie pobiera Firefoksa... pobierz i Ty! firefox.pl
--
To unsubscribe from this list: send the line "unsubscribe linux-msdos" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [v2 3/7] x86/mpx, x86/insn: Relocate insn util functions to a new insn-utils

[Ricardo Neri]

On Sun, 2016-12-25 at 15:17 +0900, Masami Hiramatsu wrote:
> Hi Ricado,
> 
> On Fri, 23 Dec 2016 17:37:41 -0800
> Ricardo Neri  wrote:
> 
> > Other kernel submodules can benefit from using the utility functions
> > defined in mpx.c to obtain the addresses and values of operands contained
> > in the general purpose registers. An instance of this is the emulation code
> > used for instructions protected by the Intel User-Mode Instruction
> > Prevention feature.
> > 
> > Thus, these functions are relocated to a new insn-utils.c file. The reason
> > to not relocate these utilities for insn.c is that the latter solely
> > analyses instructions given by a struct insn. The file insn-utils.c intends
> > to be used when, for instance, determining addresses from the contents
> > of the general purpose registers.
> > 
> > To avoid creating a new insn-utils.h, insn.h is used. One caveat, however,
> > is that several #include's were needed by the utility functions.
> > 
> > Functions are simply relocated. There are not functional or indentation
> > changes.
> 
> Thank you for your great work! :)

Thanks a lot for taking the time to review!
> 
> > ---
> >  arch/x86/include/asm/insn.h |   6 ++
> >  arch/x86/lib/Makefile   |   2 +-
> >  arch/x86/lib/insn-utils.c   | 148 
> > 
> >  arch/x86/mm/mpx.c   | 136 +---
> >  4 files changed, 156 insertions(+), 136 deletions(-)
> >  create mode 100644 arch/x86/lib/insn-utils.c
> > 
> > diff --git a/arch/x86/include/asm/insn.h b/arch/x86/include/asm/insn.h
> > index b3e32b0..9dc9d42 100644
> > --- a/arch/x86/include/asm/insn.h
> > +++ b/arch/x86/include/asm/insn.h
> > @@ -22,6 +22,10 @@
> >  
> >  /* insn_attr_t is defined in inat.h */
> >  #include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> >  
> >  struct insn_field {
> > union {
> > @@ -106,6 +110,8 @@ extern void insn_get_sib(struct insn *insn);
> >  extern void insn_get_displacement(struct insn *insn);
> >  extern void insn_get_immediate(struct insn *insn);
> >  extern void insn_get_length(struct insn *insn);
> > +extern void __user *insn_get_addr_ref(struct insn *insn, struct pt_regs 
> > *regs);
> > +extern int get_reg_offset_rm(struct insn *insn, struct pt_regs *regs);
> 
> Could you also rename this to add "insn_" prefix?

Sure. This should have the prefix. I missed this.
> 
> Other part looks good to me :)
> (btw, I saw a kbuild bot warning, would you also test it with
>  CONFIG_X86_DECODER_SELFTEST=y?)

I will do.

Thanks and BR,
Ricardo
> 
> Thanks again!
> 
> >  
> >  /* Attribute will be determined after getting ModRM (for opcode groups) */
> >  static inline void insn_get_attribute(struct insn *insn)
> > diff --git a/arch/x86/lib/Makefile b/arch/x86/lib/Makefile
> > index 34a7413..0d01d82 100644
> > --- a/arch/x86/lib/Makefile
> > +++ b/arch/x86/lib/Makefile
> > @@ -23,7 +23,7 @@ lib-y := delay.o misc.o cmdline.o cpu.o
> >  lib-y += usercopy_$(BITS).o usercopy.o getuser.o putuser.o
> >  lib-y += memcpy_$(BITS).o
> >  lib-$(CONFIG_RWSEM_XCHGADD_ALGORITHM) += rwsem.o
> > -lib-$(CONFIG_INSTRUCTION_DECODER) += insn.o inat.o
> > +lib-$(CONFIG_INSTRUCTION_DECODER) += insn.o inat.o insn-utils.o
> >  lib-$(CONFIG_RANDOMIZE_BASE) += kaslr.o
> >  
> >  obj-y += msr.o msr-reg.o msr-reg-export.o hweight.o
> > diff --git a/arch/x86/lib/insn-utils.c b/arch/x86/lib/insn-utils.c
> > new file mode 100644
> > index 000..598bbd6
> > --- /dev/null
> > +++ b/arch/x86/lib/insn-utils.c
> > @@ -0,0 +1,148 @@
> > +/*
> > + * Utility functions for x86 operand and address decoding
> > + *
> > + * Copyright (C) Intel Corporation 2016
> > + */
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +
> > +enum reg_type {
> > +   REG_TYPE_RM = 0,
> > +   REG_TYPE_INDEX,
> > +   REG_TYPE_BASE,
> > +};
> > +
> > +static int get_reg_offset(struct insn *insn, struct pt_regs *regs,
> > + enum reg_type type)
> > +{
> > +   int regno = 0;
> > +
> > +   static const int regoff[] = {
> > +   offsetof(struct pt_regs, ax),
> > +   offsetof(struct pt_regs, cx),
> > +   offsetof(struct pt_regs, dx),
> > +   offsetof(struct pt_regs, bx),
> > +   offsetof(struct pt_regs, sp),
> > +   offsetof(struct pt_regs, bp),
> > +   offsetof(struct pt_regs, si),
> > +   offsetof(struct pt_regs, di),
> > +#ifdef CONFIG_X86_64
> > +   offsetof(struct pt_regs, r8),
> > +   offsetof(struct pt_regs, r9),
> > +   offsetof(struct pt_regs, r10),
> > +   offsetof(struct pt_regs, r11),
> > +   offsetof(struct pt_regs, r12),
> > +   offsetof(struct pt_regs, r13),
> > +   offsetof(struct pt_regs, r14),
> > +   offsetof(struct pt_regs, r15),
> > +#endif
> > +   };
> > +   int nr_registers = ARRAY_SIZE(regoff);
> > +   /*
> > +* Don't possibly decode a 32-bit instructions 

Re: [v2 6/7] x86/traps: Fixup general protection faults caused by UMIP

[Ricardo Neri]

On Fri, 2016-12-23 at 18:11 -0800, Andy Lutomirski wrote:
> On Fri, Dec 23, 2016 at 5:37 PM, Ricardo Neri
>  wrote:
> > If the User-Mode Instruction Prevention CPU feature is available and
> > enabled, a general protection fault will be issued if the instructions
> > sgdt, sldt, sidt, str or smsw are executed from user-mode context
> > (CPL > 0). If the fault was caused by any of the instructions protected
> > by UMIP, fixup_umip_exceptino will emulate dummy results for these
> > instructions.
> >
> > Cc: Andy Lutomirski 
> > Cc: Andrew Morton 
> > Cc: H. Peter Anvin 
> > Cc: Borislav Petkov 
> > Cc: Brian Gerst 
> > Cc: Chen Yucong 
> > Cc: Chris Metcalf 
> > Cc: Dave Hansen 
> > Cc: Fenghua Yu 
> > Cc: Huang Rui 
> > Cc: Jiri Slaby 
> > Cc: Jonathan Corbet 
> > Cc: Michael S. Tsirkin 
> > Cc: Paul Gortmaker 
> > Cc: Peter Zijlstra 
> > Cc: Ravi V. Shankar 
> > Cc: Shuah Khan 
> > Cc: Vlastimil Babka 
> > Cc: Tony Luck 
> > Cc: Paolo Bonzini 
> > Cc: Liang Z. Li 
> > Cc: Alexandre Julliard 
> > Cc: Stas Sergeev 
> > Cc: x...@kernel.org
> > Cc: linux-msdos@vger.kernel.org
> > Signed-off-by: Ricardo Neri 
> > ---
> >  arch/x86/kernel/traps.c | 4 
> >  1 file changed, 4 insertions(+)
> >
> > diff --git a/arch/x86/kernel/traps.c b/arch/x86/kernel/traps.c
> > index bf0c6d0..5044fb3 100644
> > --- a/arch/x86/kernel/traps.c
> > +++ b/arch/x86/kernel/traps.c
> > @@ -64,6 +64,7 @@
> >  #include 
> >  #include 
> >  #include 
> > +#include 
> >
> >  #ifdef CONFIG_X86_64
> >  #include 
> > @@ -491,6 +492,9 @@ do_general_protection(struct pt_regs *regs, long 
> > error_code)
> > RCU_LOCKDEP_WARN(!rcu_is_watching(), "entry code didn't wake RCU");
> > cond_local_irq_enable(regs);
> >
> > +   if (user_mode(regs) && !fixup_umip_exception(regs))
> > +   return;
> > +
> 
> I would do fixup_umip_exception(regs) == 0 to make it more obvious
> what's going on.

Sure. I will make this change.
> 
> Also, since you're allowing this in v8086 mode, I think this should
> have an explicit test in
> tools/testing/selftests/x86/entry_from_vm86.c.  I *think* it will work
> fine, but the pt_regs handling in vm86 mode is quite scary and has
> been rewritten recently.

I will include a test for this.

Thanks and BR,
Ricardo


--
To unsubscribe from this list: send the line "unsubscribe linux-msdos" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [v2 1/7] x86/mpx: Do not use SIB index if index points to R/ESP

[Ricardo Neri]

On Fri, 2016-12-23 at 17:57 -0800, Andy Lutomirski wrote:
> On Fri, Dec 23, 2016 at 5:37 PM, Ricardo Neri
>  wrote:
> > Section 2.2.1.2 of the Intel 64 and IA-32 Architectures Software
> > Developer's Manual volume 2A states that when memory addressing is used
> > (i.e., mod part of ModR/M is not 3), a SIB byte is used and the index of
> > the SIB byte points to the R/ESP (i.e.,index = 4), the index should not be
> > used in the computation of the memory address.
> >
> > An example of such instruction could be
> >
> > insn -0x80(%rsp)
> >
> > This is represented as:
> >
> >  [opcode] 4c 24 80
> >
> >   ModR/M: mod: 1, reg: 1: r/m: 4 (R/ESP)
> >   SIB 24: sc: 0, index: 100 (R/ESP), base(R/ESP): 100
> >   Displacement -0x80
> >
> > The correct address is (base) + displacement; no index is used.
> >
> > Care is taken to allow R12 to be used as index, which is a valid scenario.
> 
> Since I have no idea what this patch has to do with the rest of the
> series, I'll ask a question:

Thanks for your feedback! I saw in a previous e-mail that you read the
cover-letter. :)
> 
> Why isn't this code in the standard x86 instruction decoder?  Is the
> decoder similarly buggy?

I did not find any bug in the instruction decoder. I think the reason
this code is not in the decoder is that the decoder only gives you the
bytes of the instructions without any meaning. For instance, it gives
you the ModRM byte but it does not tell you what register or addressing
mode is used.

To fully emulate the UMIP instructions I need to give meaning to the
ModRM and SIB bytes. Since I was trying many operand combinations, I ran
into this issue.

Thanks and BR,
Ricardo


--
To unsubscribe from this list: send the line "unsubscribe linux-msdos" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html