Re: [PATCH v4 01/17] x86/mpx: Do not use SIB index if index points to R/ESP

[Ricardo Neri]

On Fri, 2017-02-24 at 09:47 -0500, Nathan Howard wrote:
> Also, this code would read better with the inner test
> reversed or done first
> 
> if (indx_offset < 0) {
> if (indx_offset != -EDOM)
> goto out_err;
> indx = 0;
> } else {
> indx = regs_get_register(etc...)
> }
> 
> or
> if (indx_offset == -EDOM)
> indx = 0;
> else if (indx_offset < 0)
> goto err;
> 
> 
> Or goto out_err;
> 
> 
> else
> indx = regs_get_register(etc...)
> 
> The compiler should generate the same code in any
> case, but either could improve reader understanding.
> 
> 
> Also, it may be a tweak more efficient to handle the most likely
> runtime case in the conditional stack first (whichever that may be).

The most likely case will be a positive value but I need to check for
negatives first :( I could wrap the first conditional in an "unlikely".

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 v4 01/17] x86/mpx: Do not use SIB index if index points to R/ESP

[Adan Hawthorn]

On Thu, Feb 23, 2017 at 9:33 PM, Joe Perches  wrote:
> On Thu, 2017-02-23 at 14:17 -0800, Ricardo Neri wrote:
>> On Thu, 2017-02-23 at 08:24 +0100, Peter Zijlstra wrote:
>> > On Wed, Feb 22, 2017 at 10:36:50PM -0800, Ricardo Neri wrote:
>> > > + /*
>> > > +  * 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 < 0)
>> > > - goto out_err;
>> > > + if (indx_offset == -EDOM)
>> > > + indx = 0;
>> > > + else
>> > > + goto out_err;
>> > > + else
>> > > + indx = regs_get_register(regs, indx_offset);
>> >
>> > Kernel coding style requires more brackets than are strictly required by
>> > C, any block longer than 1 line needs then. Also, if one leg of a
>> > conditional needs them, then they should be on both legs.
>> >
>> > Your code has many such instances, please change them all.
>>
>> Will do. Sorry for the noise. These instances escaped the checkpatch
>> script.
>
> Also, this code would read better with the inner test
> reversed or done first
>
> if (indx_offset < 0) {
> if (indx_offset != -EDOM)
> goto out_err;
> indx = 0;
> } else {
> indx = regs_get_register(etc...)
> }
>
> or
> if (indx_offset == -EDOM)
> indx = 0;
> else if (indx_offset < 0)
> goto err;

Or, goto out_err;

> else
> indx = regs_get_register(etc...)
>
> The compiler should generate the same code in any
> case, but either could improve reader understanding.

Also, it may be a tweak more efficient to handle the most likely
runtime case in the conditional stack first (whichever that may be).
--
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 v4 01/17] x86/mpx: Do not use SIB index if index points to R/ESP

[Joe Perches]

On Thu, 2017-02-23 at 14:17 -0800, Ricardo Neri wrote:
> On Thu, 2017-02-23 at 08:24 +0100, Peter Zijlstra wrote:
> > On Wed, Feb 22, 2017 at 10:36:50PM -0800, Ricardo Neri wrote:
> > > + /*
> > > +  * 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 < 0)
> > > - goto out_err;
> > > + if (indx_offset == -EDOM)
> > > + indx = 0;
> > > + else
> > > + goto out_err;
> > > + else
> > > + indx = regs_get_register(regs, indx_offset);
> > 
> > Kernel coding style requires more brackets than are strictly required by
> > C, any block longer than 1 line needs then. Also, if one leg of a
> > conditional needs them, then they should be on both legs.
> > 
> > Your code has many such instances, please change them all.
> 
> Will do. Sorry for the noise. These instances escaped the checkpatch
> script.

Also, this code would read better with the inner test
reversed or done first

if (indx_offset < 0) {
if (indx_offset != -EDOM)
goto out_err;
indx = 0;
} else {
indx = regs_get_register(etc...)
}

or
if (indx_offset == -EDOM)
indx = 0;
else if (indx_offset < 0)
goto err;
else
indx = regs_get_register(etc...)

The compiler should generate the same code in any
case, but either could improve reader understanding.
--
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 v4 01/17] x86/mpx: Do not use SIB index if index points to R/ESP

[Ricardo Neri]

On Thu, 2017-02-23 at 18:33 -0800, Joe Perches wrote:
> On Thu, 2017-02-23 at 14:17 -0800, Ricardo Neri wrote:
> > On Thu, 2017-02-23 at 08:24 +0100, Peter Zijlstra wrote:
> > > On Wed, Feb 22, 2017 at 10:36:50PM -0800, Ricardo Neri wrote:
> > > > +   /*
> > > > +* 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 < 0)
> > > > -   goto out_err;
> > > > +   if (indx_offset == -EDOM)
> > > > +   indx = 0;
> > > > +   else
> > > > +   goto out_err;
> > > > +   else
> > > > +   indx = regs_get_register(regs, 
> > > > indx_offset);
> > > 
> > > Kernel coding style requires more brackets than are strictly required by
> > > C, any block longer than 1 line needs then. Also, if one leg of a
> > > conditional needs them, then they should be on both legs.
> > > 
> > > Your code has many such instances, please change them all.
> > 
> > Will do. Sorry for the noise. These instances escaped the checkpatch
> > script.
> 
> Also, this code would read better with the inner test
> reversed or done first
> 
>   if (indx_offset < 0) {
>   if (indx_offset != -EDOM)
>   goto out_err;
>   indx = 0;
>   } else {
>   indx = regs_get_register(etc...)
>   }
> 
> or
>   if (indx_offset == -EDOM)
>   indx = 0;
>   else if (indx_offset < 0)
>   goto err;
>   else
>   indx = regs_get_register(etc...)
> 
> The compiler should generate the same code in any
> case, but either could improve reader understanding.

I agree! I will change it with your clever solution.

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 v4 01/17] x86/mpx: Do not use SIB index if index points to R/ESP

[Ricardo Neri]

On Thu, 2017-02-23 at 08:24 +0100, Peter Zijlstra wrote:
> On Wed, Feb 22, 2017 at 10:36:50PM -0800, Ricardo Neri wrote:
> > +   /*
> > +* 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 < 0)
> > -   goto out_err;
> > +   if (indx_offset == -EDOM)
> > +   indx = 0;
> > +   else
> > +   goto out_err;
> > +   else
> > +   indx = regs_get_register(regs, indx_offset);
> 
> Kernel coding style requires more brackets than are strictly required by
> C, any block longer than 1 line needs then. Also, if one leg of a
> conditional needs them, then they should be on both legs.
> 
> Your code has many such instances, please change them all.

Will do. Sorry for the noise. These instances escaped the checkpatch
script.

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

Coding style vs legibility [was Re: [PATCH v4 01/17] x86/mpx: Do not use SIB index if index points to R/ESP]

[Mouse]

> Certainly if you have code with an odd mix of styles it is much
> harder to read, and ultimately source code is for *humans* to
> understand.  So enforcing a consistent style, even if it is not your
> own style, makes it much easier to follow!

It can.  It doesn't always.

I've yet to see a coding style rule that can't profitably be broken in
at least a few cases ("profitably" here meaning, the breaking actually
improves rather than impairs readability).  Truly did Emerson write
that "[a] foolish consistency is the hobgoblin of little minds".

Readability is a fundamentally subjective thing, after all, and thus
brings all of the human layer's messy inconsistency with it.

/~\ The ASCII Mouse
\ / Ribbon Campaign
 X  Against HTMLmo...@rodents-montreal.org
/ \ Email!   7D C8 61 52 5D E7 2D 39  4E F1 31 3E E8 B3 27 4B
--
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 v4 01/17] x86/mpx: Do not use SIB index if index points to R/ESP

[Paul Crawford]

On 23/02/17 07:24, Peter Zijlstra wrote:

On Wed, Feb 22, 2017 at 10:36:50PM -0800, Ricardo Neri wrote:

+   /*
+* 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 < 0)
-   goto out_err;
+   if (indx_offset == -EDOM)
+   indx = 0;
+   else
+   goto out_err;
+   else
+   indx = regs_get_register(regs, indx_offset);


Kernel coding style requires more brackets than are strictly required by
C, any block longer than 1 line needs then. Also, if one leg of a
conditional needs them, then they should be on both legs.

Your code has many such instances, please change them all.


This was one issue raised with the OpenSSL review where a lack of 
brackets made some coding errors less obvious and in the changes was 
down as applying "KNF format the whole source tree"


Probably the easiest fix is to use some tool like "indent --linux-style" 
though I am not completely sure that option is perfect.


Certainly if you have code with an odd mix of styles it is much harder 
to read, and ultimately source code is for *humans* to understand. So 
enforcing a consistent style, even if it is not your own style, makes it 
much easier to follow!


Just my 2p worth.

Regard,
Paul
--
 Dr. Paul S. Crawford
 c/o Satellite Station
 University of Dundee
 Small's Wynd, Dundee, DD1 4HN
 Email: p...@sat.dundee.ac.uk
 Tel: +44 (0)1382 38 4687
 The University of Dundee is a Scottish Registered Charity, No. SC015096
--
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 v4 01/17] x86/mpx: Do not use SIB index if index points to R/ESP

[Peter Zijlstra]

On Wed, Feb 22, 2017 at 10:36:50PM -0800, Ricardo Neri wrote:
> + /*
> +  * 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 < 0)
> - goto out_err;
> + if (indx_offset == -EDOM)
> + indx = 0;
> + else
> + goto out_err;
> + else
> + indx = regs_get_register(regs, indx_offset);

Kernel coding style requires more brackets than are strictly required by
C, any block longer than 1 line needs then. Also, if one leg of a
conditional needs them, then they should be on both legs.

Your code has many such instances, please change them all.
--
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 v4 01/17] x86/mpx: Do not use SIB index if index points to R/ESP

[Ricardo Neri]

Section 2.2.1.2 of the Intel 64 and IA-32 Architectures Software
Developer's Manual volume 2A states that when memory addressing is used
(i.e., mod part of ModR/M is not 3), a SIB byte is used and the index of
the SIB byte points to the R/ESP (i.e., index = 4), the index should not be
used in the computation of the memory address.

In these cases the address is simply the value present in the register
pointed by the base part of the SIB byte plus the displacement byte.

An example of such instruction could be

insn -0x80(%rsp)

This is represented as:

 [opcode] 4c 23 80

  ModR/M=0x4c: mod: 0x1, reg: 0x1: r/m: 0x4(R/ESP)
  SIB=0x23: sc: 0, index: 0x100(R/ESP), base: 0x11(R/EBX):
  Displacement -0x80

The correct address is (base) + displacement; no index is used.

We can achieve the desired effect of not using the index by making
get_reg_offset return -EDOM in this particular case. This value indicates
callers that they should not use the index to calculate the address.
EINVAL continues to indicate that an error when decoding the SIB byte.

Care is taken to allow R12 to be used as index, which is a valid scenario.

Cc: Dave Hansen 
Cc: Adam Buchbinder 
Cc: Colin Ian King 
Cc: Lorenzo Stoakes 
Cc: Qiaowei Ren 
Cc: Ravi V. Shankar 
Cc: x...@kernel.org
Signed-off-by: Ricardo Neri 
---
 arch/x86/mm/mpx.c | 20 ++--
 1 file changed, 18 insertions(+), 2 deletions(-)

diff --git a/arch/x86/mm/mpx.c b/arch/x86/mm/mpx.c
index 86c2d96..6a034bc 100644
--- a/arch/x86/mm/mpx.c
+++ b/arch/x86/mm/mpx.c
@@ -110,6 +110,13 @@ static int get_reg_offset(struct insn *insn, struct 
pt_regs *regs,
regno = X86_SIB_INDEX(insn->sib.value);
if (X86_REX_X(insn->rex_prefix.value))
regno += 8;
+   /*
+* If mod !=3, register R/ESP (regno=4) is not used as index in
+* the address computation. Check is done after looking at REX.X
+* This is because R12 (regno=12) can be used as an index.
+*/
+   if (regno == 4 && X86_MODRM_MOD(insn->modrm.value) != 3)
+   return -EDOM;
break;
 
case REG_TYPE_BASE:
@@ -158,11 +165,20 @@ static void __user *mpx_get_addr_ref(struct insn *insn, 
struct pt_regs *regs)
goto out_err;
 
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 < 0)
-   goto out_err;
+   if (indx_offset == -EDOM)
+   indx = 0;
+   else
+   goto out_err;
+   else
+   indx = regs_get_register(regs, indx_offset);
 
base = regs_get_register(regs, base_offset);
-   indx = regs_get_register(regs, indx_offset);
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