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 05/26] x86/mpx: Do not use SIB.base if its value is 101b and ModRM.mod = 0

[Borislav Petkov]

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."

> +  */
> + if (!X86_MODRM_MOD(insn->modrm.value) && regno == 5)
> + return -EDOM;
> +
>   if (X86_REX_B(insn->rex_prefix.value))
>   regno += 8;
>   break;

-- 
Regards/Gruss,
Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.
--
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

[PATCH v7 05/26] x86/mpx: Do not use SIB.base if its value is 101b and ModRM.mod = 0

[Ricardo Neri]

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.
+*/
+   if (!X86_MODRM_MOD(insn->modrm.value) && regno == 5)
+   return -EDOM;
+
if (X86_REX_B(insn->rex_prefix.value))
regno += 8;
break;
@@ -162,16 +171,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 (base_offset == -EDOM)
+   base = 0;
+   else if (base_offset < 0)
goto out_err;
+   else
+   base = regs_get_register(regs, base_offset);
 
indx_offset = get_reg_offset(insn, regs, 
REG_TYPE_INDEX);
-   /*
-* A negative offset generally means a error, except
-* -EDOM, which means that the contents of the register
-* should not be used as index.
-*/
if (indx_offset == -EDOM)
indx = 0;
else if (indx_offset < 0)
@@ -179,7 +193,6 @@ static void __user *mpx_get_addr_ref(struct insn *insn, 
struct pt_regs *regs)
else
indx = regs_get_register(regs, indx_offset);
 
-   base = regs_get_register(regs, base_offset);
eff_addr = base + indx * (1 << X86_SIB_SCALE(sib));
} else {
addr_offset = get_reg_offset(insn, regs, REG_TYPE_RM);
-- 
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