Re: [v6 PATCH 04/21] x86/mpx, x86/insn: Relocate insn util functions to a new insn-kernel

2017-04-25 Thread Ricardo Neri 
On Wed, 2017-04-12 at 12:03 +0200, Borislav Petkov wrote:
> > +  * If mod is 0 and register R/EBP (regno=5) is
> indicated in the
> > +  * base part of the SIB byte, the value of such
> register should
> > +  * not be used in the address computation. Also, a
> 32-bit
> > +  * displacement is expected in this case; the
> instruction
> > +  * decoder takes care of it. This is true for both R13
> and
> > +  * R/EBP as REX.B will not be decoded.
> > +  */
> > + if (regno == 5 && X86_MODRM_MOD(insn->modrm.value) ==
> 0)
> > + return -EDOM;
> > +
> > + if (X86_REX_B(insn->rex_prefix.value))
> > + regno += 8;
> > + break;
> > +
> > + default:
> > + pr_err("invalid register type");
> > + BUG();
> 
> WARNING: Avoid crashing the kernel - try using WARN_ON & recovery code
> rather than BUG() or BUG_ON()
> #211: FILE: arch/x86/lib/insn-eval.c:90:
> +   BUG();
> 
> And checkpatch is kinda right. We need to warn here, not explode. Oh
> and
> that function returns negative values on error...
> 
> Please change that with a patch ontop of the move.

Sure, I will change it.


--
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 03/21] x86/mpx: Do not use R/EBP as base in the SIB byte with Mod = 0

2017-04-25 Thread Ricardo Neri 
On Wed, 2017-04-12 at 00:08 +0200, Borislav Petkov wrote:
> On Tue, Mar 07, 2017 at 04:32:36PM -0800, 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 to R/EBP (i.e., base = 5) and the mod part
> > of the ModRM byte is zero, the value of such register will not be used
> > as part of the address computation. To signal this, a -EDOM error is
> > returned to indicate callers that they should ignore the value.
> > 
> > Also, for this particular case, a displacement of 32-bits should follow
> > the SIB byte if the mod part of ModRM is equal to zero. The instruction
> > decoder ensures that this is the case.
> > 
> > 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 | 29 ++---
> >  1 file changed, 22 insertions(+), 7 deletions(-)
> > 
> > diff --git a/arch/x86/mm/mpx.c b/arch/x86/mm/mpx.c
> > index d9e92d6..ef7eb67 100644
> > --- a/arch/x86/mm/mpx.c
> > +++ b/arch/x86/mm/mpx.c
> > @@ -121,6 +121,17 @@ static int get_reg_offset(struct insn *insn, struct 
> > pt_regs *regs,
> >  
> > case REG_TYPE_BASE:
> > regno = X86_SIB_BASE(insn->sib.value);
> > +   /*
> > +* If mod is 0 and register R/EBP (regno=5) is indicated in the
> > +* base part of the SIB byte,
> 
> you can simply say here: "if SIB.base == 5, the base of the
> register-indirect addressing is 0."

This is better wording. I will change it.
> 
> > the value of such register should
> > +* not be used in the address computation. Also, a 32-bit
> 
> Not "Also" but "In this case, a 32-bit displacement..."

Will change.
> 
> > +* displacement is expected in this case; the instruction
> > +* decoder takes care of it. This is true for both R13 and
> > +* R/EBP as REX.B will not be decoded.
> 
> You don't need that sentence as the only thing that matters is ModRM.mod
> being 0.

For the specific case of ModRM.mod being 0, I feel I need to clarify
that REX.B is not decoded and if SIB.base is %r13 the base is also 0.
This comment adds clarity because REX.X is decoded when determining
SIB.index.
> 
> > +*/
> > +   if (regno == 5 && X86_MODRM_MOD(insn->modrm.value) == 0)
> 
> The 0 test we normally do with the ! (also flip parts of if-condition):
> 
>   if (!X86_MODRM_MOD(insn->modrm.value) && regno == 5)

Will change it.
> 
> > +   return -EDOM;
> > +
> > if (X86_REX_B(insn->rex_prefix.value))
> > regno += 8;
> > break;
> > @@ -161,16 +172,21 @@ static void __user *mpx_get_addr_ref(struct insn 
> > *insn, struct pt_regs *regs)
> > eff_addr = regs_get_register(regs, addr_offset);
> > } else {
> > if (insn->sib.nbytes) {
> > +   /*
> > +* Negative values in the base and index offset means
> > +* an error when decoding the SIB byte. Except -EDOM,
> > +* which means that the registers should not be used
> > +* in the address computation.
> > +*/
> > base_offset = get_reg_offset(insn, regs, REG_TYPE_BASE);
> > -   if (base_offset < 0)
> > +   if (unlikely(base_offset == -EDOM))
> > +   base = 0;
> > +   else if (unlikely(base_offset < 0))
> 
> Bah, unlikely's in something which is not really a hot path. They only
> encumber readability, no need for them.

I will remove them.

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: [v6 PATCH 01/21] x86/mpx: Use signed variables to compute effective addresses

2017-04-25 Thread Ricardo Neri 
On Tue, 2017-04-11 at 23:56 +0200, Borislav Petkov wrote:
> On Tue, Mar 07, 2017 at 04:32:34PM -0800, Ricardo Neri wrote:
> > Even though memory addresses are unsigned. The operands used to compute the
> 
>   ... unsigned, the operands ...

Oops! I will correct.


--
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-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))
> +