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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20170411113126.ygq52fdoe6rp6jzw@pd.tnic>
Date:   Tue, 11 Apr 2017 13:31:26 +0200
From:   Borislav Petkov <bp@...e.de>
To:     Ricardo Neri <ricardo.neri-calderon@...ux.intel.com>
Cc:     Ingo Molnar <mingo@...hat.com>,
        Thomas Gleixner <tglx@...utronix.de>,
        "H. Peter Anvin" <hpa@...or.com>,
        Andy Lutomirski <luto@...nel.org>,
        Peter Zijlstra <peterz@...radead.org>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Brian Gerst <brgerst@...il.com>,
        Chris Metcalf <cmetcalf@...lanox.com>,
        Dave Hansen <dave.hansen@...ux.intel.com>,
        Paolo Bonzini <pbonzini@...hat.com>,
        Liang Z Li <liang.z.li@...el.com>,
        Masami Hiramatsu <mhiramat@...nel.org>,
        Huang Rui <ray.huang@....com>, Jiri Slaby <jslaby@...e.cz>,
        Jonathan Corbet <corbet@....net>,
        "Michael S. Tsirkin" <mst@...hat.com>,
        Paul Gortmaker <paul.gortmaker@...driver.com>,
        Vlastimil Babka <vbabka@...e.cz>,
        Chen Yucong <slaoub@...il.com>,
        Alexandre Julliard <julliard@...ehq.org>,
        Stas Sergeev <stsp@...t.ru>, Fenghua Yu <fenghua.yu@...el.com>,
        "Ravi V. Shankar" <ravi.v.shankar@...el.com>,
        Shuah Khan <shuah@...nel.org>, linux-kernel@...r.kernel.org,
        x86@...nel.org, linux-msdos@...r.kernel.org, wine-devel@...ehq.org,
        Adam Buchbinder <adam.buchbinder@...il.com>,
        Colin Ian King <colin.king@...onical.com>,
        Lorenzo Stoakes <lstoakes@...il.com>,
        Qiaowei Ren <qiaowei.ren@...el.com>,
        Nathan Howard <liverlint@...il.com>,
        Adan Hawthorn <adanhawthorn@...il.com>,
        Joe Perches <joe@...ches.com>
Subject: Re: [v6 PATCH 02/21] x86/mpx: Do not use SIB index if index points
 to R/ESP

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