Re: [v3 PATCH 04/10] x86/insn-kernel: Add a function to obtain register offset in ModRM

2017-01-31 Thread Ricardo Neri 
On Fri, 2017-01-27 at 16:53 +0900, Masami Hiramatsu wrote:
> On Wed, 25 Jan 2017 22:07:16 -0800
> Ricardo Neri  wrote:
> 
> > Hi Masami,
> > 
> > On Thu, 2017-01-26 at 11:11 +0900, Masami Hiramatsu wrote:
> > > On Wed, 25 Jan 2017 12:23:47 -0800
> > > Ricardo Neri  wrote:
> > > 
> > > > The function insn_get_reg_offset requires a type to indicate whether
> > > > the returned offset is that given by by the ModRM or the SIB byte.
> > > > Callers of this function would need the definition of the type struct.
> > > > This is not needed. Instead, auxiliary functions can be defined for
> > > > this purpose.
> > > > 
> > > > When the operand is a register, the emulation code for User-Mode
> > > > Instruction Prevention needs to know the offset of the register 
> > > > indicated
> > > > in the r/m part of the ModRM byte. Thus, start by adding an auxiliary
> > > > function for this purpose.
> > > 
> > > Hmm, why wouldn't you just rename it to insn_get_reg_offset() and export 
> > > it?
> > 
> > Do you mean exporting the structure that I mention above? The problem
> > that I am trying to solve is that callers sometimes want to know the
> > offset of the register encoded in the SiB or the ModRM bytes. I could
> > use something 
> > 
> > insn_get_reg_offset(insn, regs, INSN_TYPE_MODRM)
> > insn_get_reg_offset(insn, regs, INSN_TYPE_SIB)
> > 
> > Instead, I opted for
> > 
> > insn_get_reg_offset_rm(insn, regs)
> > insn_get_reg_offset_sib(insn, regs)
> > 
> > to avoid exposing an enum with the INSN_TYPE_MODRM, INSN_TYPE_SIB.
> 
> OK, if so, I think you should export both of them at once, not only
> insn_get_reg_offset_rm().

Sure, I will include both functions.

Thanks and BR,
Ricardo
> 
> Thank you,
> 
> > 
> > If you feel that the former makes more sense, I can change the
> > implementation.
> > 
> > Thanks and BR,
> > Ricardo
> > > 
> > > Thank you,
> > > 
> > > > 
> > > > 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-kernel.h | 1 +
> > > >  arch/x86/lib/insn-kernel.c | 5 +
> > > >  2 files changed, 6 insertions(+)
> > > > 
> > > > diff --git a/arch/x86/include/asm/insn-kernel.h 
> > > > b/arch/x86/include/asm/insn-kernel.h
> > > > index aef416a..3f34649 100644
> > > > --- a/arch/x86/include/asm/insn-kernel.h
> > > > +++ b/arch/x86/include/asm/insn-kernel.h
> > > > @@ -12,5 +12,6 @@
> > > >  #include 
> > > >  
> > > >  void __user *insn_get_addr_ref(struct insn *insn, struct pt_regs 
> > > > *regs);
> > > > +int insn_get_reg_offset_rm(struct insn *insn, struct pt_regs *regs);
> > > >  
> > > >  #endif /* _ASM_X86_INSN_KERNEL_H */
> > > > diff --git a/arch/x86/lib/insn-kernel.c b/arch/x86/lib/insn-kernel.c
> > > > index 8072abe..267cab4 100644
> > > > --- a/arch/x86/lib/insn-kernel.c
> > > > +++ b/arch/x86/lib/insn-kernel.c
> > > > @@ -95,6 +95,11 @@ static int get_reg_offset(struct insn *insn, struct 
> > > > pt_regs *regs,
> > > > return regoff[regno];
> > > >  }
> > > >  
> > > > +int insn_get_reg_offset_rm(struct insn *insn, struct pt_regs *regs)
> > > > +{
> > > > +   return get_reg_offset(insn, regs, REG_TYPE_RM);
> > > > +}
> > > > +
> > > >  /*
> > > >   * return the address being referenced be instruction
> > > >   * for rm=3 returning the content of the rm reg
> > > > -- 
> > > > 2.9.3
> > > > 
> > > 
> > > 
> > 
> > 
> 
> 


--
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: [v3 PATCH 04/10] x86/insn-kernel: Add a function to obtain register offset in ModRM

2017-01-26 Thread Masami Hiramatsu 
On Wed, 25 Jan 2017 22:07:16 -0800
Ricardo Neri  wrote:

> Hi Masami,
> 
> On Thu, 2017-01-26 at 11:11 +0900, Masami Hiramatsu wrote:
> > On Wed, 25 Jan 2017 12:23:47 -0800
> > Ricardo Neri  wrote:
> > 
> > > The function insn_get_reg_offset requires a type to indicate whether
> > > the returned offset is that given by by the ModRM or the SIB byte.
> > > Callers of this function would need the definition of the type struct.
> > > This is not needed. Instead, auxiliary functions can be defined for
> > > this purpose.
> > > 
> > > When the operand is a register, the emulation code for User-Mode
> > > Instruction Prevention needs to know the offset of the register indicated
> > > in the r/m part of the ModRM byte. Thus, start by adding an auxiliary
> > > function for this purpose.
> > 
> > Hmm, why wouldn't you just rename it to insn_get_reg_offset() and export it?
> 
> Do you mean exporting the structure that I mention above? The problem
> that I am trying to solve is that callers sometimes want to know the
> offset of the register encoded in the SiB or the ModRM bytes. I could
> use something 
> 
> insn_get_reg_offset(insn, regs, INSN_TYPE_MODRM)
> insn_get_reg_offset(insn, regs, INSN_TYPE_SIB)
> 
> Instead, I opted for
> 
> insn_get_reg_offset_rm(insn, regs)
> insn_get_reg_offset_sib(insn, regs)
> 
> to avoid exposing an enum with the INSN_TYPE_MODRM, INSN_TYPE_SIB.

OK, if so, I think you should export both of them at once, not only
insn_get_reg_offset_rm().

Thank you,

> 
> If you feel that the former makes more sense, I can change the
> implementation.
> 
> Thanks and BR,
> Ricardo
> > 
> > Thank you,
> > 
> > > 
> > > 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-kernel.h | 1 +
> > >  arch/x86/lib/insn-kernel.c | 5 +
> > >  2 files changed, 6 insertions(+)
> > > 
> > > diff --git a/arch/x86/include/asm/insn-kernel.h 
> > > b/arch/x86/include/asm/insn-kernel.h
> > > index aef416a..3f34649 100644
> > > --- a/arch/x86/include/asm/insn-kernel.h
> > > +++ b/arch/x86/include/asm/insn-kernel.h
> > > @@ -12,5 +12,6 @@
> > >  #include 
> > >  
> > >  void __user *insn_get_addr_ref(struct insn *insn, struct pt_regs *regs);
> > > +int insn_get_reg_offset_rm(struct insn *insn, struct pt_regs *regs);
> > >  
> > >  #endif /* _ASM_X86_INSN_KERNEL_H */
> > > diff --git a/arch/x86/lib/insn-kernel.c b/arch/x86/lib/insn-kernel.c
> > > index 8072abe..267cab4 100644
> > > --- a/arch/x86/lib/insn-kernel.c
> > > +++ b/arch/x86/lib/insn-kernel.c
> > > @@ -95,6 +95,11 @@ static int get_reg_offset(struct insn *insn, struct 
> > > pt_regs *regs,
> > >   return regoff[regno];
> > >  }
> > >  
> > > +int insn_get_reg_offset_rm(struct insn *insn, struct pt_regs *regs)
> > > +{
> > > + return get_reg_offset(insn, regs, REG_TYPE_RM);
> > > +}
> > > +
> > >  /*
> > >   * return the address being referenced be instruction
> > >   * for rm=3 returning the content of the rm reg
> > > -- 
> > > 2.9.3
> > > 
> > 
> > 
> 
> 


-- 
Masami Hiramatsu 
--
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: [v3 PATCH 04/10] x86/insn-kernel: Add a function to obtain register offset in ModRM

2017-01-25 Thread Ricardo Neri 
Hi Masami,

On Thu, 2017-01-26 at 11:11 +0900, Masami Hiramatsu wrote:
> On Wed, 25 Jan 2017 12:23:47 -0800
> Ricardo Neri  wrote:
> 
> > The function insn_get_reg_offset requires a type to indicate whether
> > the returned offset is that given by by the ModRM or the SIB byte.
> > Callers of this function would need the definition of the type struct.
> > This is not needed. Instead, auxiliary functions can be defined for
> > this purpose.
> > 
> > When the operand is a register, the emulation code for User-Mode
> > Instruction Prevention needs to know the offset of the register indicated
> > in the r/m part of the ModRM byte. Thus, start by adding an auxiliary
> > function for this purpose.
> 
> Hmm, why wouldn't you just rename it to insn_get_reg_offset() and export it?

Do you mean exporting the structure that I mention above? The problem
that I am trying to solve is that callers sometimes want to know the
offset of the register encoded in the SiB or the ModRM bytes. I could
use something 

insn_get_reg_offset(insn, regs, INSN_TYPE_MODRM)
insn_get_reg_offset(insn, regs, INSN_TYPE_SIB)

Instead, I opted for

insn_get_reg_offset_rm(insn, regs)
insn_get_reg_offset_sib(insn, regs)

to avoid exposing an enum with the INSN_TYPE_MODRM, INSN_TYPE_SIB.

If you feel that the former makes more sense, I can change the
implementation.

Thanks and BR,
Ricardo
> 
> Thank you,
> 
> > 
> > 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-kernel.h | 1 +
> >  arch/x86/lib/insn-kernel.c | 5 +
> >  2 files changed, 6 insertions(+)
> > 
> > diff --git a/arch/x86/include/asm/insn-kernel.h 
> > b/arch/x86/include/asm/insn-kernel.h
> > index aef416a..3f34649 100644
> > --- a/arch/x86/include/asm/insn-kernel.h
> > +++ b/arch/x86/include/asm/insn-kernel.h
> > @@ -12,5 +12,6 @@
> >  #include 
> >  
> >  void __user *insn_get_addr_ref(struct insn *insn, struct pt_regs *regs);
> > +int insn_get_reg_offset_rm(struct insn *insn, struct pt_regs *regs);
> >  
> >  #endif /* _ASM_X86_INSN_KERNEL_H */
> > diff --git a/arch/x86/lib/insn-kernel.c b/arch/x86/lib/insn-kernel.c
> > index 8072abe..267cab4 100644
> > --- a/arch/x86/lib/insn-kernel.c
> > +++ b/arch/x86/lib/insn-kernel.c
> > @@ -95,6 +95,11 @@ static int get_reg_offset(struct insn *insn, struct 
> > pt_regs *regs,
> > return regoff[regno];
> >  }
> >  
> > +int insn_get_reg_offset_rm(struct insn *insn, struct pt_regs *regs)
> > +{
> > +   return get_reg_offset(insn, regs, REG_TYPE_RM);
> > +}
> > +
> >  /*
> >   * return the address being referenced be instruction
> >   * for rm=3 returning the content of the rm reg
> > -- 
> > 2.9.3
> > 
> 
> 


--
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: [v3 PATCH 04/10] x86/insn-kernel: Add a function to obtain register offset in ModRM

2017-01-25 Thread Masami Hiramatsu 
On Wed, 25 Jan 2017 12:23:47 -0800
Ricardo Neri  wrote:

> The function insn_get_reg_offset requires a type to indicate whether
> the returned offset is that given by by the ModRM or the SIB byte.
> Callers of this function would need the definition of the type struct.
> This is not needed. Instead, auxiliary functions can be defined for
> this purpose.
> 
> When the operand is a register, the emulation code for User-Mode
> Instruction Prevention needs to know the offset of the register indicated
> in the r/m part of the ModRM byte. Thus, start by adding an auxiliary
> function for this purpose.

Hmm, why wouldn't you just rename it to insn_get_reg_offset() and export it?

Thank you,

> 
> 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-kernel.h | 1 +
>  arch/x86/lib/insn-kernel.c | 5 +
>  2 files changed, 6 insertions(+)
> 
> diff --git a/arch/x86/include/asm/insn-kernel.h 
> b/arch/x86/include/asm/insn-kernel.h
> index aef416a..3f34649 100644
> --- a/arch/x86/include/asm/insn-kernel.h
> +++ b/arch/x86/include/asm/insn-kernel.h
> @@ -12,5 +12,6 @@
>  #include 
>  
>  void __user *insn_get_addr_ref(struct insn *insn, struct pt_regs *regs);
> +int insn_get_reg_offset_rm(struct insn *insn, struct pt_regs *regs);
>  
>  #endif /* _ASM_X86_INSN_KERNEL_H */
> diff --git a/arch/x86/lib/insn-kernel.c b/arch/x86/lib/insn-kernel.c
> index 8072abe..267cab4 100644
> --- a/arch/x86/lib/insn-kernel.c
> +++ b/arch/x86/lib/insn-kernel.c
> @@ -95,6 +95,11 @@ static int get_reg_offset(struct insn *insn, struct 
> pt_regs *regs,
>   return regoff[regno];
>  }
>  
> +int insn_get_reg_offset_rm(struct insn *insn, struct pt_regs *regs)
> +{
> + return get_reg_offset(insn, regs, REG_TYPE_RM);
> +}
> +
>  /*
>   * return the address being referenced be instruction
>   * for rm=3 returning the content of the rm reg
> -- 
> 2.9.3
> 


-- 
Masami Hiramatsu 
--
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