Re: [v6 PATCH 02/21] x86/mpx: Do not use SIB index if index points to R/ESP

[Borislav Petkov]

On Tue, Mar 07, 2017 at 04:32:35PM -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 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 <dave.hansen@xxxxxxxxxxxxxxx>
> Cc: Adam Buchbinder <adam.buchbinder@xxxxxxxxx>
> Cc: Colin Ian King <colin.king@xxxxxxxxxxxxx>
> Cc: Lorenzo Stoakes <lstoakes@xxxxxxxxx>
> Cc: Qiaowei Ren <qiaowei.ren@xxxxxxxxx>
> Cc: Peter Zijlstra <peterz@xxxxxxxxxxxxx>
> Cc: Nathan Howard <liverlint@xxxxxxxxx>
> Cc: Adan Hawthorn <adanhawthorn@xxxxxxxxx>
> Cc: Joe Perches <joe@xxxxxxxxxxx>
> Cc: Ravi V. Shankar <ravi.v.shankar@xxxxxxxxx>
> Cc: x86@xxxxxxxxxx
> Signed-off-by: Ricardo Neri <ricardo.neri-calderon@xxxxxxxxxxxxxxx>
> ---
>  arch/x86/mm/mpx.c | 19 +++++++++++++++++--
>  1 file changed, 17 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/x86/mm/mpx.c b/arch/x86/mm/mpx.c
> index ff112e3..d9e92d6 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;

Hmm, ok, so this is a bit confusing, to me at least. Maybe you're saying
the same things but here's how I see it:

1. When ModRM.mod != 11b and ModRM.rm == 100b, all that does mean
is that you have a SIB byte following. I.e., you have indexed
register-indirect addressing.

Now, you still need to decode the SIB byte and it goes this way:

SIB.index == 100b means that the index register specification is
null, i.e., the scale*index portion of that indexed register-indirect
addressing is null, i.e., you have an offset following the SIB byte.
Now, depending on ModRM.mod, that offset is:

ModRM.mod == 01b -> 1 byte offset
ModRM.mod == 10b -> 4 bytes offset

That's why for an instruction like this one (let's use your example) you
have:

	8b 4c 23 80             mov    -0x80(%rbx,%riz,1),%ecx

That's basically a binutils hack to state that the SIB index register is
null.

Another SIB index register works, of course:

	 8b 4c 03 80             mov -0x80(%rbx,%rax,1),%ecx

Ok, so far so good.

2. Now, the %r12 thing is part of the REX implications to those
encodings: That's the REX.X bit which adds a fourth bit to the encoding
of the SIB base register, i.e., if you specify a register with
SIB.index, you want to be able to specify all 16 regs, thus the 4th
bit. That's why it says that the SIB byte is required for %r12-based
addressing.

I.e., you can still have a SIB.index == 100b addressing with an index
register which is not null but that is only because SIB.index is now
{REX.X=1b, 100b}, i.e.:

Prefixes:
 REX:                   0x43 { 4 [w]: 0 [r]: 0 [x]: 1 [b]: 1 }
Opcode:                 0x8b
ModRM:                  0x4c  [mod:1b][.R:0b,reg:1b][.B:1b,r/m:1100b]
                        register-indirect mode, 1-byte offset in displ. field
SIB:                    0x63 [.B:1b,base:1011b][.X:1b,idx:1100b][scale: 1]

 MOV Gv,Ev; MOV reg{16,32,64} reg/mem{16,32,64}
               0:       43 8b 4c 63 80          mov -0x80(%r11,%r12,2),%ecx

So, I'm not saying your version is necessarily wrong - I'm just saying
that it could explain the situation a bit more verbose.

Btw, I'd flip the if-test above:

	if (X86_MODRM_MOD(insn->modrm.value) != 3 && regno == 4)

to make it just like the order the conditions are specified in the
manuals.

Thanks.

-- 
Regards/Gruss,
    Boris.

SUSE Linux GmbH, GF: Felix ImendÃrffer, Jane Smithard, Graham Norton, HRB 21284 (AG NÃrnberg)
--