Re: [v6 PATCH 12/21] x86/insn: Support both signed 32-bit and 64-bit effective addresses

2017-05-11 Thread Ricardo Neri 
On Mon, 2017-05-08 at 13:42 +0200, Borislav Petkov wrote:
> On Wed, Apr 26, 2017 at 08:33:46PM -0700, Ricardo Neri wrote:
> > This is the reason I check the value of long_bytes. If long_bytes is not
> > 4, being the only other possible value 8 (perhaps I need to issue an
> > error when the value is not any of these values),
> 
> Well, maybe I'm a bit too paranoid. Bottom line is, we should do the
> address computations exactly like the hardware does them so that there
> are no surprises. Doing them with longs looks ok to me.

Using long is exactly what I intend to do. The problem that I am trying
to resolve is to sign-extend signed memory offsets of 32-bit programs
running on 64-bit kernels. For 64-bit programs running on 64-bit kernels
I can simply use longs. I added error checking in my v7 of this series
[1].

Thanks and BR,
Ricardo

[1]. https://lkml.org/lkml/2017/5/5/407


--
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: [v6 PATCH 12/21] x86/insn: Support both signed 32-bit and 64-bit effective addresses

2017-05-08 Thread Borislav Petkov 
On Wed, Apr 26, 2017 at 08:33:46PM -0700, Ricardo Neri wrote:
> This is the reason I check the value of long_bytes. If long_bytes is not
> 4, being the only other possible value 8 (perhaps I need to issue an
> error when the value is not any of these values),

Well, maybe I'm a bit too paranoid. Bottom line is, we should do the
address computations exactly like the hardware does them so that there
are no surprises. Doing them with longs looks ok to me.

-- 
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: [v6 PATCH 12/21] x86/insn: Support both signed 32-bit and 64-bit effective addresses

2017-04-26 Thread Ricardo Neri 
On Tue, 2017-04-25 at 15:51 +0200, Borislav Petkov wrote:
> On Tue, Mar 07, 2017 at 04:32:45PM -0800, Ricardo Neri wrote:
> > The 32-bit and 64-bit address encodings are identical. This means that we
> > can use the same function in both cases. In order to reuse the function for
> > 32-bit address encodings, we must sign-extend our 32-bit signed operands to
> > 64-bit signed variables (only for 64-bit builds). To decide on whether sign
> > extension is needed, we rely on the address size as given by the
> > instruction structure.
> > 
> > Lastly, before computing the linear address, we must truncate our signed
> > 64-bit signed effective address if the address size is 32-bit.
> > 
> > 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/lib/insn-eval.c | 44 
> >  1 file changed, 32 insertions(+), 12 deletions(-)
> > 
> > diff --git a/arch/x86/lib/insn-eval.c b/arch/x86/lib/insn-eval.c
> > index edb360f..a9a1704 100644
> > --- a/arch/x86/lib/insn-eval.c
> > +++ b/arch/x86/lib/insn-eval.c
> > @@ -559,6 +559,15 @@ int insn_get_reg_offset_sib_index(struct insn *insn, 
> > struct pt_regs *regs)
> > return get_reg_offset(insn, regs, REG_TYPE_INDEX);
> >  }
> >  
> > +static inline long __to_signed_long(unsigned long val, int long_bytes)
> > +{
> > +#ifdef CONFIG_X86_64
> > +   return long_bytes == 4 ? (long)((int)((val) & 0x)) : (long)val;
> 
> I don't think this always works as expected:
> 
> ---
> typedef unsigned int u32;
> typedef unsigned long u64;
> 
> int main()
> {
> u64 v = 0x1;
> 
> printf("v: %ld, 0x%lx, %ld\n", v, v, (long)((int)((v) & 0x)));
> 
> return 0;
> }
> --
> ...
> 
> v: 8589934591, 0x1, -1
> 
> Now, this should not happen on 32-bit because unsigned long is 32-bit
> there but can that happen on 64-bit?

This is the reason I check the value of long_bytes. If long_bytes is not
4, being the only other possible value 8 (perhaps I need to issue an
error when the value is not any of these values), the cast is simply
(long)val. I modified your test program with:

printf("v: %ld, 0x%lx, %ld, %ld\n", v, v, (long)((int)((v) &
0x)), (long)v);

and I get:

v: 8589934591, 0x1, -1, 8589934591.

Am I missing something?

> 
> > +#else
> > +   return (long)val;
> > +#endif
> > +}
> > +
> >  /*
> >   * return the address being referenced be instruction
> >   * for rm=3 returning the content of the rm reg
> > @@ -567,19 +576,21 @@ int insn_get_reg_offset_sib_index(struct insn *insn, 
> > struct pt_regs *regs)
> >  void __user *insn_get_addr_ref(struct insn *insn, struct pt_regs *regs)
> >  {
> > unsigned long linear_addr, seg_base_addr;
> > -   long eff_addr, base, indx;
> > -   int addr_offset, base_offset, indx_offset;
> > +   long eff_addr, base, indx, tmp;
> > +   int addr_offset, base_offset, indx_offset, addr_bytes;
> > insn_byte_t sib;
> >  
> > insn_get_modrm(insn);
> > insn_get_sib(insn);
> > sib = insn->sib.value;
> > +   addr_bytes = insn->addr_bytes;
> >  
> > if (X86_MODRM_MOD(insn->modrm.value) == 3) {
> > addr_offset = get_reg_offset(insn, regs, REG_TYPE_RM);
> > if (addr_offset < 0)
> > goto out_err;
> > -   eff_addr = regs_get_register(regs, addr_offset);
> > +   tmp = regs_get_register(regs, addr_offset);
> > +   eff_addr = __to_signed_long(tmp, addr_bytes);
> 
> This repeats throughout the function so it begs to be a separate:
> 
>   get_mem_addr()
> 
> or so.

Yes, the same pattern is used in all places except when using register
operands (ModRM.rm == 11b). I will look into putting it in a function.
> 
> > seg_base_addr = insn_get_seg_base(regs, insn, addr_offset,
> >   false);
> > } else {
> > @@ -591,20 +602,24 @@ void __user *insn_get_addr_ref(struct insn *insn, 
> > struct pt_regs *regs)
> >  * in the address computation.
> >  */
> > base_offset = get_reg_offset(insn, regs, REG_TYPE_BASE);
> > -   if (unlikely(base_offset == -EDOM))
> > +   if (unlikely(base_offset == -EDOM)) {
> > base = 0;
> > -

Re: [v6 PATCH 12/21] x86/insn: Support both signed 32-bit and 64-bit effective addresses

2017-04-25 Thread Borislav Petkov 
On Tue, Mar 07, 2017 at 04:32:45PM -0800, Ricardo Neri wrote:
> The 32-bit and 64-bit address encodings are identical. This means that we
> can use the same function in both cases. In order to reuse the function for
> 32-bit address encodings, we must sign-extend our 32-bit signed operands to
> 64-bit signed variables (only for 64-bit builds). To decide on whether sign
> extension is needed, we rely on the address size as given by the
> instruction structure.
> 
> Lastly, before computing the linear address, we must truncate our signed
> 64-bit signed effective address if the address size is 32-bit.
> 
> 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/lib/insn-eval.c | 44 
>  1 file changed, 32 insertions(+), 12 deletions(-)
> 
> diff --git a/arch/x86/lib/insn-eval.c b/arch/x86/lib/insn-eval.c
> index edb360f..a9a1704 100644
> --- a/arch/x86/lib/insn-eval.c
> +++ b/arch/x86/lib/insn-eval.c
> @@ -559,6 +559,15 @@ int insn_get_reg_offset_sib_index(struct insn *insn, 
> struct pt_regs *regs)
>   return get_reg_offset(insn, regs, REG_TYPE_INDEX);
>  }
>  
> +static inline long __to_signed_long(unsigned long val, int long_bytes)
> +{
> +#ifdef CONFIG_X86_64
> + return long_bytes == 4 ? (long)((int)((val) & 0x)) : (long)val;

I don't think this always works as expected:

---
typedef unsigned int u32;
typedef unsigned long u64;

int main()
{
u64 v = 0x1;

printf("v: %ld, 0x%lx, %ld\n", v, v, (long)((int)((v) & 0x)));

return 0;
}
--
...

v: 8589934591, 0x1, -1

Now, this should not happen on 32-bit because unsigned long is 32-bit
there but can that happen on 64-bit?

> +#else
> + return (long)val;
> +#endif
> +}
> +
>  /*
>   * return the address being referenced be instruction
>   * for rm=3 returning the content of the rm reg
> @@ -567,19 +576,21 @@ int insn_get_reg_offset_sib_index(struct insn *insn, 
> struct pt_regs *regs)
>  void __user *insn_get_addr_ref(struct insn *insn, struct pt_regs *regs)
>  {
>   unsigned long linear_addr, seg_base_addr;
> - long eff_addr, base, indx;
> - int addr_offset, base_offset, indx_offset;
> + long eff_addr, base, indx, tmp;
> + int addr_offset, base_offset, indx_offset, addr_bytes;
>   insn_byte_t sib;
>  
>   insn_get_modrm(insn);
>   insn_get_sib(insn);
>   sib = insn->sib.value;
> + addr_bytes = insn->addr_bytes;
>  
>   if (X86_MODRM_MOD(insn->modrm.value) == 3) {
>   addr_offset = get_reg_offset(insn, regs, REG_TYPE_RM);
>   if (addr_offset < 0)
>   goto out_err;
> - eff_addr = regs_get_register(regs, addr_offset);
> + tmp = regs_get_register(regs, addr_offset);
> + eff_addr = __to_signed_long(tmp, addr_bytes);

This repeats throughout the function so it begs to be a separate:

get_mem_addr()

or so.

>   seg_base_addr = insn_get_seg_base(regs, insn, addr_offset,
> false);
>   } else {
> @@ -591,20 +602,24 @@ void __user *insn_get_addr_ref(struct insn *insn, 
> struct pt_regs *regs)
>* in the address computation.
>*/
>   base_offset = get_reg_offset(insn, regs, REG_TYPE_BASE);
> - if (unlikely(base_offset == -EDOM))
> + if (unlikely(base_offset == -EDOM)) {
>   base = 0;
> - else if (unlikely(base_offset < 0))
> + } else if (unlikely(base_offset < 0)) {
>   goto out_err;
> - else
> - base = regs_get_register(regs, base_offset);
> + } else {
> + tmp = regs_get_register(regs, base_offset);
> + base = __to_signed_long(tmp, addr_bytes);
> + }
>  
>   indx_offset = get_reg_offset(insn, regs, 
> REG_TYPE_INDEX);
> - if (unlikely(indx_offset == -EDOM))
> + if (unlikely(indx_offset == -EDOM)) {
>   indx = 0;
> - else if (unlikely(indx_offset < 0))
> +