Re: [PATCH v7 07/26] x86/insn-eval: Do not BUG on invalid register type

[Ricardo Neri]

On Tue, 2017-06-06 at 13:58 +0200, Borislav Petkov wrote:
> On Mon, Jun 05, 2017 at 11:06:58PM -0700, Ricardo Neri wrote:
> > I agree that insn-eval reads somewhat funny. I did not want to go with
> > insn-dec.c as insn.c, in my opinion, already decodes the instruction
> > (i.e., it finds prefixes, opcodes, ModRM, SIB and displacement bytes).
> > In insn-eval.c I simply take those decoded parameters and evaluate them
> > to obtain the values they contain (e.g., a specific memory location).
> > Perhaps, insn-resolve.c could be a better name? Or maybe isnn-operands?
> 
> So actually I'm gravitating towards calling all that instruction
> "massaging" code with a single prefix to denote this comes from the insn
> decoder/handler/whatever...
> 
> I.e.,
> 
>   "insn-decoder: x86: invalid register type"
> 
> or
> 
>   "inat: x86: invalid register type"
> 
> or something to that effect.
> 
> I mean, If we're going to grow our own - as we do, apparently - maybe it
> all should be a separate entity with its proper name.

I see. You were more concerned about the naming of the coding artifacts
(e.g., function names, error prints, etc) than the actual filenames. I
think I have aligned with the function naming of insn.c in all the
functions that are exposed via header by using the inns_ prefix. For
static functions I don't use that prefix. Perhaps I can use the __
prefix as insn.c does.

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: [PATCH v7 09/26] x86/insn-eval: Add utility function to identify string instructions

[Borislav Petkov]

On Mon, Jun 05, 2017 at 11:01:21PM -0700, Ricardo Neri wrote:
> If I was to leave out string instructions from this function it should
> be renamed as is_string_instruction_non_lods_outs. In my opinion this
> separation makes the code more clear and I would end up having logic to
> decide which segment register to use in two places. Does it makes sense
> to you?

Ok, sure.

Thanks.

-- 
Regards/Gruss,
Boris.

SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 
(AG Nürnberg)
-- 
--
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: [PATCH v7 07/26] x86/insn-eval: Do not BUG on invalid register type

[Borislav Petkov]

On Mon, Jun 05, 2017 at 11:06:58PM -0700, Ricardo Neri wrote:
> I agree that insn-eval reads somewhat funny. I did not want to go with
> insn-dec.c as insn.c, in my opinion, already decodes the instruction
> (i.e., it finds prefixes, opcodes, ModRM, SIB and displacement bytes).
> In insn-eval.c I simply take those decoded parameters and evaluate them
> to obtain the values they contain (e.g., a specific memory location).
> Perhaps, insn-resolve.c could be a better name? Or maybe isnn-operands?

So actually I'm gravitating towards calling all that instruction
"massaging" code with a single prefix to denote this comes from the insn
decoder/handler/whatever...

I.e.,

"insn-decoder: x86: invalid register type"

or

"inat: x86: invalid register type"

or something to that effect.

I mean, If we're going to grow our own - as we do, apparently - maybe it
all should be a separate entity with its proper name.

Hmm.

-- 
Regards/Gruss,
Boris.

SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 
(AG Nürnberg)
-- 
--
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: [PATCH v7 05/26] x86/mpx: Do not use SIB.base if its value is 101b and ModRM.mod = 0

[Ricardo Neri]

On Mon, 2017-05-29 at 15:07 +0200, Borislav Petkov wrote:
> On Fri, May 05, 2017 at 11:17:03AM -0700, 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 a SIB byte is used and the
> > base of the SIB byte points is base = 101b and the mod part
> > of the ModRM byte is zero, the base port on the effective address
> > computation is null. In this case, a 32-bit displacement follows the SIB
> > byte. This is obtained when the instruction decoder parses the operands.
> > 
> > To signal this scenario, a -EDOM error is returned to indicate callers that
> > they should ignore the base.
> > 
> > Cc: Borislav Petkov 
> > Cc: Andy Lutomirski 
> > Cc: Dave Hansen 
> > Cc: Adam Buchbinder 
> > Cc: Colin Ian King 
> > Cc: Lorenzo Stoakes 
> > Cc: Qiaowei Ren 
> > Cc: Peter Zijlstra 
> > Cc: Nathan Howard 
> > Cc: Adan Hawthorn 
> > Cc: Joe Perches 
> > Cc: Ravi V. Shankar 
> > Cc: x...@kernel.org
> > Signed-off-by: Ricardo Neri 
> > ---
> >  arch/x86/mm/mpx.c | 27 ---
> >  1 file changed, 20 insertions(+), 7 deletions(-)
> > 
> > diff --git a/arch/x86/mm/mpx.c b/arch/x86/mm/mpx.c
> > index 7397b81..30aef92 100644
> > --- a/arch/x86/mm/mpx.c
> > +++ b/arch/x86/mm/mpx.c
> > @@ -122,6 +122,15 @@ static int get_reg_offset(struct insn *insn, struct 
> > pt_regs *regs,
> >  
> > case REG_TYPE_BASE:
> > regno = X86_SIB_BASE(insn->sib.value);
> > +   /*
> > +* If ModRM.mod is 0 and SIB.base == 5, the base of the
> > +* register-indirect addressing is 0. In this case, a
> > +* 32-bit displacement is expected in this case; the
> > +* instruction decoder finds such displacement for us.
> 
> That last sentence reads funny. Just say:
> 
> "In this case, a 32-bit displacement follows the SIB byte."

Agreed. I will update the comment to make more sense.

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: [PATCH v7 07/26] x86/insn-eval: Do not BUG on invalid register type

[Ricardo Neri]

On Mon, 2017-05-29 at 18:37 +0200, Borislav Petkov wrote:
> On Fri, May 05, 2017 at 11:17:05AM -0700, Ricardo Neri wrote:
> > We are not in a critical failure path. The invalid register type is caused
> > when trying to decode invalid instruction bytes from a user-space program.
> > Thus, simply print an error message. To prevent this warning from being
> > abused from user space programs, use the rate-limited variant of printk.
> > 
> > Cc: Borislav Petkov 
> > Cc: Andy Lutomirski 
> > Cc: Dave Hansen 
> > Cc: Adam Buchbinder 
> > Cc: Colin Ian King 
> > Cc: Lorenzo Stoakes 
> > Cc: Qiaowei Ren 
> > Cc: Arnaldo Carvalho de Melo 
> > Cc: Masami Hiramatsu 
> > Cc: Adrian Hunter 
> > Cc: Kees Cook 
> > Cc: Thomas Garnier 
> > Cc: Peter Zijlstra 
> > Cc: Dmitry Vyukov 
> > Cc: Ravi V. Shankar 
> > Cc: x...@kernel.org
> > Signed-off-by: Ricardo Neri 
> > ---
> >  arch/x86/lib/insn-eval.c | 6 +++---
> >  1 file changed, 3 insertions(+), 3 deletions(-)
> > 
> > diff --git a/arch/x86/lib/insn-eval.c b/arch/x86/lib/insn-eval.c
> > index e746a6f..182e2ae 100644
> > --- a/arch/x86/lib/insn-eval.c
> > +++ b/arch/x86/lib/insn-eval.c
> > @@ -5,6 +5,7 @@
> >   */
> >  #include 
> >  #include 
> > +#include 
> >  #include 
> >  #include 
> >  #include 
> > @@ -85,9 +86,8 @@ static int get_reg_offset(struct insn *insn, struct 
> > pt_regs *regs,
> > break;
> >  
> > default:
> > -   pr_err("invalid register type");
> > -   BUG();
> > -   break;
> > +   printk_ratelimited(KERN_ERR "insn-eval: x86: invalid register 
> > type");
> 
> You can use pr_err_ratelimited() and define "insn-eval" with pr_fmt.
> Look for examples in the tree.

Will do. I have looked at the examples.
> 
> Btw, "insn-eval" is perhaps not the right name - since we're building
> an instruction decoder, maybe it should be called "insn-dec" or so. I'm
> looking at those other arch/x86/lib/insn.c, arch/x86/include/asm/inat.h
> things and how they're starting to morph into one decoding facility,
> AFAICT.

I agree that insn-eval reads somewhat funny. I did not want to go with
insn-dec.c as insn.c, in my opinion, already decodes the instruction
(i.e., it finds prefixes, opcodes, ModRM, SIB and displacement bytes).
In insn-eval.c I simply take those decoded parameters and evaluate them
to obtain the values they contain (e.g., a specific memory location).
Perhaps, insn-resolve.c could be a better name? Or maybe isnn-operands?

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: [PATCH v7 08/26] x86/insn-eval: Add a utility function to get register offsets

[Ricardo Neri]

On Mon, 2017-05-29 at 19:16 +0200, Borislav Petkov wrote:
> On Fri, May 05, 2017 at 11:17:06AM -0700, Ricardo Neri wrote:
> > The function get_reg_offset() returns the offset to the register the
> > argument specifies as indicated in an enumeration of type offset. Callers
> > of this function would need the definition of such enumeration. This is
> > not needed. Instead, add helper functions for this purpose. These functions
> > are useful in cases when, for instance, the caller needs to decide whether
> > the operand is a register or a memory location by looking at the rm part
> > of the ModRM byte. As of now, this is the only helper function that is
> > needed.
> > 
> > Cc: Dave Hansen 
> > Cc: Adam Buchbinder 
> > Cc: Colin Ian King 
> > Cc: Lorenzo Stoakes 
> > Cc: Qiaowei Ren 
> > Cc: Arnaldo Carvalho de Melo 
> > Cc: Masami Hiramatsu 
> > Cc: Adrian Hunter 
> > Cc: Kees Cook 
> > Cc: Thomas Garnier 
> > Cc: Peter Zijlstra 
> > Cc: Borislav Petkov 
> > Cc: Dmitry Vyukov 
> > Cc: Ravi V. Shankar 
> > Cc: x...@kernel.org
> > Signed-off-by: Ricardo Neri 
> > ---
> >  arch/x86/include/asm/insn-eval.h |  1 +
> >  arch/x86/lib/insn-eval.c | 15 +++
> >  2 files changed, 16 insertions(+)
> > 
> > diff --git a/arch/x86/include/asm/insn-eval.h 
> > b/arch/x86/include/asm/insn-eval.h
> > index 5cab1b1..7e8c963 100644
> > --- a/arch/x86/include/asm/insn-eval.h
> > +++ b/arch/x86/include/asm/insn-eval.h
> > @@ -12,5 +12,6 @@
> >  #include 
> >  
> >  void __user *insn_get_addr_ref(struct insn *insn, struct pt_regs *regs);
> > +int insn_get_modrm_rm_off(struct insn *insn, struct pt_regs *regs);
> >  
> >  #endif /* _ASM_X86_INSN_EVAL_H */
> > diff --git a/arch/x86/lib/insn-eval.c b/arch/x86/lib/insn-eval.c
> > index 182e2ae..8b16761 100644
> > --- a/arch/x86/lib/insn-eval.c
> > +++ b/arch/x86/lib/insn-eval.c
> > @@ -97,6 +97,21 @@ static int get_reg_offset(struct insn *insn, struct 
> > pt_regs *regs,
> > return regoff[regno];
> >  }
> >  
> > +/**
> > + * insn_get_reg_offset_modrm_rm() - Obtain register in r/m part of ModRM 
> > byte
> 
> That name needs to be synced with the function name below.

Ugh! I missed this. I will update accordingly. Thanks for the detailed
review.

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: [PATCH v7 09/26] x86/insn-eval: Add utility function to identify string instructions

[Ricardo Neri]

On Mon, 2017-05-29 at 23:48 +0200, Borislav Petkov wrote:
> On Fri, May 05, 2017 at 11:17:07AM -0700, Ricardo Neri wrote:
> > String instructions are special because in protected mode, the linear
> > address is always obtained via the ES segment register in operands that
> > use the (E)DI register.
> 
>  ... and DS for rSI.

Right, I omitted this in the commit message.
> 
> If we're going to account for both operands of string instructions with
> two operands.
> 
> Btw, LODS and OUTS use only DS:rSI as a source operand. So we have to be
> careful with the generalization here. So if ES:rDI is the only seg. reg
> we want, then we don't need to look at those insns... (we assume DS by
> default).

My intention with this function is to write a function that does only
one thing: identify string instructions, irrespective of the operands
they use. A separate function, resolve_seg_register, will have the logic
to decide what to segment register to use based on the registers used as
operands, whether we are looking at a string instruction, whether we
have segment override prefixes and whether such overrides should be
ignored.

If I was to leave out string instructions from this function it should
be renamed as is_string_instruction_non_lods_outs. In my opinion this
separation makes the code more clear and I would end up having logic to
decide which segment register to use in two places. Does it makes sense
to you?

> 
> ...
> 
> > +/**
> > + * is_string_instruction - Determine if instruction is a string instruction
> > + * @insn:  Instruction structure containing the opcode
> > + *
> > + * Return: true if the instruction, determined by the opcode, is any of the
> > + * string instructions as defined in the Intel Software Development manual.
> > + * False otherwise.
> > + */
> > +static bool is_string_instruction(struct insn *insn)
> > +{
> > +   insn_get_opcode(insn);
> > +
> > +   /* all string instructions have a 1-byte opcode */
> > +   if (insn->opcode.nbytes != 1)
> > +   return false;
> > +
> > +   switch (insn->opcode.bytes[0]) {
> > +   case INSB:
> > +   /* fall through */
> > +   case INSW_INSD:
> > +   /* fall through */
> > +   case OUTSB:
> > +   /* fall through */
> > +   case OUTSW_OUTSD:
> > +   /* fall through */
> > +   case MOVSB:
> > +   /* fall through */
> > +   case MOVSW_MOVSD:
> > +   /* fall through */
> > +   case CMPSB:
> > +   /* fall through */
> > +   case CMPSW_CMPSD:
> > +   /* fall through */
> > +   case STOSB:
> > +   /* fall through */
> > +   case STOSW_STOSD:
> > +   /* fall through */
> > +   case LODSB:
> > +   /* fall through */
> > +   case LODSW_LODSD:
> > +   /* fall through */
> > +   case SCASB:
> > +   /* fall through */
> 
> That "fall through" for every opcode is just too much. Also, you can use
> the regularity of the x86 opcode space and do:
> 
>   case 0x6c ... 0x6f: /* INS/OUTS */
>   case 0xa4 ... 0xa7: /* MOVS/CMPS */
>   case 0xaa ... 0xaf: /* STOS/LODS/SCAS */
>   return true;
>   default:
>   return false;
> }
> 
> And voila, there's your compact is_string_insn() function! :^)

Thanks for the suggestion! It looks really nice. I will implement
accordingly.

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