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

Borislav Petkov
2017-04-11T11:31:26Z

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@linux.intel.com>
> Cc: Adam Buchbinder <adam.buchbinder@gmail.com>
> Cc: Colin Ian King <colin.king@canonical.com>
> Cc: Lorenzo Stoakes <lstoakes@gmail.com>
> Cc: Qiaowei Ren <qiaowei.ren@intel.com>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: Nathan Howard <liverlint@gmail.com>
> Cc: Adan Hawthorn <adanhawthorn@gmail.com>
> Cc: Joe Perches <joe@perches.com>
> Cc: Ravi V. Shankar <ravi.v.shankar@intel.com>
> Cc: x86@kernel.org
> Signed-off-by: Ricardo Neri <ricardo.neri-calderon@linux.intel.com>
> ---
> 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)
--