[PATCH] uprobes/x86: fix rip-relative handling of EVEX-encoded insns.

[PATCH] uprobes/x86: fix rip-relative handling of EVEX-encoded insns.

[Denys Vlasenko]

Since instruction decoder now supports EVEX-encoded insns, two fixes are needed
to correctly handle them in uprobes.

Extended bits for MODRM.rm field need to be sanitized just like we do it
for VEX3, to avoid encoding wrong register for register-relative access.
EVEX has _two_ extended bits: b and x. Theoretically, EVEX.x should be
ignored by CPU (since GPRs go only up to 15, not 31), but let's be paranoid
here: proper encoding for register-relative access should have EVEX.x = 1.

Secondly, we should fetch vex.vvvv for EVEX too.
This is now super easy because insn decoder populates vex_prefix.bytes[2]
for all flavors of (e)vex encodings, even for vex2.

Signed-off-by: Denys Vlasenko <dvlasenk@redhat.com>
CC: Jim Keniston <jkenisto@us.ibm.com>
CC: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
CC: Srikar Dronamraju <srikar@linux.vnet.ibm.com>
CC: Ingo Molnar <mingo@kernel.org>
CC: Oleg Nesterov <oleg@redhat.com>
CC: x86@kernel.org
CC: linux-kernel@vger.kernel.org
---
 arch/x86/kernel/uprobes.c | 22 +++++++++++-----------
 1 file changed, 11 insertions(+), 11 deletions(-)

diff --git a/arch/x86/kernel/uprobes.c b/arch/x86/kernel/uprobes.c
index 6c1ff31..a6e6096 100644
--- a/arch/x86/kernel/uprobes.c
+++ b/arch/x86/kernel/uprobes.c
@@ -357,20 +357,22 @@ static void riprel_analyze(struct arch_uprobe *auprobe, struct insn *insn)
 		*cursor &= 0xfe;
 	}
 	/*
-	 * Similar treatment for VEX3 prefix.
-	 * TODO: add XOP/EVEX treatment when insn decoder supports them
+	 * Similar treatment for VEX3/EVEX prefix.
+	 * TODO: add XOP treatment when insn decoder supports them
 	 */
-	if (insn->vex_prefix.nbytes == 3) {
+	if (insn->vex_prefix.nbytes >= 3) {
 		/*
 		 * vex2:     c5    rvvvvLpp   (has no b bit)
 		 * vex3/xop: c4/8f rxbmmmmm wvvvvLpp
 		 * evex:     62    rxbR00mm wvvvv1pp zllBVaaa
-		 *   (evex will need setting of both b and x since
-		 *   in non-sib encoding evex.x is 4th bit of MODRM.rm)
-		 * Setting VEX3.b (setting because it has inverted meaning):
+		 * Setting VEX3.b (setting because it has inverted meaning).
+		 * Setting EVEX.x since (in non-SIB encoding) EVEX.x
+		 * is the 4th bit of MODRM.rm, and needs the same treatment.
+		 * For VEX3-encoded insns, VEX3.x value has no effect in
+		 * non-SIB encoding, the change is superfluous but harmless.
 		 */
 		cursor = auprobe->insn + insn_offset_vex_prefix(insn) + 1;
-		*cursor |= 0x20;
+		*cursor |= 0x60;
 	}
 
 	/*
@@ -415,12 +417,10 @@ static void riprel_analyze(struct arch_uprobe *auprobe, struct insn *insn)
 
 	reg = MODRM_REG(insn);	/* Fetch modrm.reg */
 	reg2 = 0xff;		/* Fetch vex.vvvv */
-	if (insn->vex_prefix.nbytes == 2)
-		reg2 = insn->vex_prefix.bytes[1];
-	else if (insn->vex_prefix.nbytes == 3)
+	if (insn->vex_prefix.nbytes)
 		reg2 = insn->vex_prefix.bytes[2];
 	/*
-	 * TODO: add XOP, EXEV vvvv reading.
+	 * TODO: add XOP vvvv reading.
 	 *
 	 * vex.vvvv field is in bits 6-3, bits are inverted.
 	 * But in 32-bit mode, high-order bit may be ignored.
-- 
2.9.2

Re: [PATCH] uprobes/x86: fix rip-relative handling of EVEX-encoded insns.

[Oleg Nesterov]

Well, I guess I have to reply...

So let me say that I am not going to even try to review this change. I know
absolutely nothing about x86 insn decoding, but I tend to trust Denys in
this area.


On 08/11, Denys Vlasenko wrote:
>
> Since instruction decoder now supports EVEX-encoded insns, two fixes are needed
> to correctly handle them in uprobes.
> 
> Extended bits for MODRM.rm field need to be sanitized just like we do it
> for VEX3, to avoid encoding wrong register for register-relative access.
> EVEX has _two_ extended bits: b and x. Theoretically, EVEX.x should be
> ignored by CPU (since GPRs go only up to 15, not 31), but let's be paranoid
> here: proper encoding for register-relative access should have EVEX.x = 1.
> 
> Secondly, we should fetch vex.vvvv for EVEX too.
> This is now super easy because insn decoder populates vex_prefix.bytes[2]
> for all flavors of (e)vex encodings, even for vex2.
> 
> Signed-off-by: Denys Vlasenko <dvlasenk@redhat.com>
> CC: Jim Keniston <jkenisto@us.ibm.com>
> CC: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
> CC: Srikar Dronamraju <srikar@linux.vnet.ibm.com>
> CC: Ingo Molnar <mingo@kernel.org>
> CC: Oleg Nesterov <oleg@redhat.com>
> CC: x86@kernel.org
> CC: linux-kernel@vger.kernel.org
> ---
>  arch/x86/kernel/uprobes.c | 22 +++++++++++-----------
>  1 file changed, 11 insertions(+), 11 deletions(-)
> 
> diff --git a/arch/x86/kernel/uprobes.c b/arch/x86/kernel/uprobes.c
> index 6c1ff31..a6e6096 100644
> --- a/arch/x86/kernel/uprobes.c
> +++ b/arch/x86/kernel/uprobes.c
> @@ -357,20 +357,22 @@ static void riprel_analyze(struct arch_uprobe *auprobe, struct insn *insn)
>  		*cursor &= 0xfe;
>  	}
>  	/*
> -	 * Similar treatment for VEX3 prefix.
> -	 * TODO: add XOP/EVEX treatment when insn decoder supports them
> +	 * Similar treatment for VEX3/EVEX prefix.
> +	 * TODO: add XOP treatment when insn decoder supports them
>  	 */
> -	if (insn->vex_prefix.nbytes == 3) {
> +	if (insn->vex_prefix.nbytes >= 3) {
>  		/*
>  		 * vex2:     c5    rvvvvLpp   (has no b bit)
>  		 * vex3/xop: c4/8f rxbmmmmm wvvvvLpp
>  		 * evex:     62    rxbR00mm wvvvv1pp zllBVaaa
> -		 *   (evex will need setting of both b and x since
> -		 *   in non-sib encoding evex.x is 4th bit of MODRM.rm)
> -		 * Setting VEX3.b (setting because it has inverted meaning):
> +		 * Setting VEX3.b (setting because it has inverted meaning).
> +		 * Setting EVEX.x since (in non-SIB encoding) EVEX.x
> +		 * is the 4th bit of MODRM.rm, and needs the same treatment.
> +		 * For VEX3-encoded insns, VEX3.x value has no effect in
> +		 * non-SIB encoding, the change is superfluous but harmless.
>  		 */
>  		cursor = auprobe->insn + insn_offset_vex_prefix(insn) + 1;
> -		*cursor |= 0x20;
> +		*cursor |= 0x60;
>  	}
>  
>  	/*
> @@ -415,12 +417,10 @@ static void riprel_analyze(struct arch_uprobe *auprobe, struct insn *insn)
>  
>  	reg = MODRM_REG(insn);	/* Fetch modrm.reg */
>  	reg2 = 0xff;		/* Fetch vex.vvvv */
> -	if (insn->vex_prefix.nbytes == 2)
> -		reg2 = insn->vex_prefix.bytes[1];
> -	else if (insn->vex_prefix.nbytes == 3)
> +	if (insn->vex_prefix.nbytes)
>  		reg2 = insn->vex_prefix.bytes[2];
>  	/*
> -	 * TODO: add XOP, EXEV vvvv reading.
> +	 * TODO: add XOP vvvv reading.
>  	 *
>  	 * vex.vvvv field is in bits 6-3, bits are inverted.
>  	 * But in 32-bit mode, high-order bit may be ignored.
> -- 
> 2.9.2
> 

Re: [PATCH] uprobes/x86: fix rip-relative handling of EVEX-encoded insns.

[Masami Hiramatsu]

On Thu, 11 Aug 2016 17:45:21 +0200
Denys Vlasenko <dvlasenk@redhat.com> wrote:

> Since instruction decoder now supports EVEX-encoded insns, two fixes are needed
> to correctly handle them in uprobes.
> 
> Extended bits for MODRM.rm field need to be sanitized just like we do it
> for VEX3, to avoid encoding wrong register for register-relative access.
> EVEX has _two_ extended bits: b and x. Theoretically, EVEX.x should be
> ignored by CPU (since GPRs go only up to 15, not 31), but let's be paranoid
> here: proper encoding for register-relative access should have EVEX.x = 1.
> 
> Secondly, we should fetch vex.vvvv for EVEX too.
> This is now super easy because insn decoder populates vex_prefix.bytes[2]
> for all flavors of (e)vex encodings, even for vex2.

OK, this was done by commit 8a764a875fe3 ("x86/asm/decoder: Create artificial
 3rd byte for 2-byte VEX") :)

OK, Looks good to me.

Acked-by: Masami Hiramatsu <mhiramat@kernel.org>

Thanks!

> 
> Signed-off-by: Denys Vlasenko <dvlasenk@redhat.com>
> CC: Jim Keniston <jkenisto@us.ibm.com>
> CC: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
> CC: Srikar Dronamraju <srikar@linux.vnet.ibm.com>
> CC: Ingo Molnar <mingo@kernel.org>
> CC: Oleg Nesterov <oleg@redhat.com>
> CC: x86@kernel.org
> CC: linux-kernel@vger.kernel.org
> ---
>  arch/x86/kernel/uprobes.c | 22 +++++++++++-----------
>  1 file changed, 11 insertions(+), 11 deletions(-)
> 
> diff --git a/arch/x86/kernel/uprobes.c b/arch/x86/kernel/uprobes.c
> index 6c1ff31..a6e6096 100644
> --- a/arch/x86/kernel/uprobes.c
> +++ b/arch/x86/kernel/uprobes.c
> @@ -357,20 +357,22 @@ static void riprel_analyze(struct arch_uprobe *auprobe, struct insn *insn)
>  		*cursor &= 0xfe;
>  	}
>  	/*
> -	 * Similar treatment for VEX3 prefix.
> -	 * TODO: add XOP/EVEX treatment when insn decoder supports them
> +	 * Similar treatment for VEX3/EVEX prefix.
> +	 * TODO: add XOP treatment when insn decoder supports them
>  	 */
> -	if (insn->vex_prefix.nbytes == 3) {
> +	if (insn->vex_prefix.nbytes >= 3) {
>  		/*
>  		 * vex2:     c5    rvvvvLpp   (has no b bit)
>  		 * vex3/xop: c4/8f rxbmmmmm wvvvvLpp
>  		 * evex:     62    rxbR00mm wvvvv1pp zllBVaaa
> -		 *   (evex will need setting of both b and x since
> -		 *   in non-sib encoding evex.x is 4th bit of MODRM.rm)
> -		 * Setting VEX3.b (setting because it has inverted meaning):
> +		 * Setting VEX3.b (setting because it has inverted meaning).
> +		 * Setting EVEX.x since (in non-SIB encoding) EVEX.x
> +		 * is the 4th bit of MODRM.rm, and needs the same treatment.
> +		 * For VEX3-encoded insns, VEX3.x value has no effect in
> +		 * non-SIB encoding, the change is superfluous but harmless.
>  		 */
>  		cursor = auprobe->insn + insn_offset_vex_prefix(insn) + 1;
> -		*cursor |= 0x20;
> +		*cursor |= 0x60;
>  	}
>  
>  	/*
> @@ -415,12 +417,10 @@ static void riprel_analyze(struct arch_uprobe *auprobe, struct insn *insn)
>  
>  	reg = MODRM_REG(insn);	/* Fetch modrm.reg */
>  	reg2 = 0xff;		/* Fetch vex.vvvv */
> -	if (insn->vex_prefix.nbytes == 2)
> -		reg2 = insn->vex_prefix.bytes[1];
> -	else if (insn->vex_prefix.nbytes == 3)
> +	if (insn->vex_prefix.nbytes)
>  		reg2 = insn->vex_prefix.bytes[2];
>  	/*
> -	 * TODO: add XOP, EXEV vvvv reading.
> +	 * TODO: add XOP vvvv reading.
>  	 *
>  	 * vex.vvvv field is in bits 6-3, bits are inverted.
>  	 * But in 32-bit mode, high-order bit may be ignored.
> -- 
> 2.9.2
> 


-- 
Masami Hiramatsu <mhiramat@kernel.org>

Re: [PATCH] uprobes/x86: fix rip-relative handling of EVEX-encoded insns.

[Srikar Dronamraju]

* Denys Vlasenko <dvlasenk@redhat.com> [2016-08-11 17:45:21]:

> Since instruction decoder now supports EVEX-encoded insns, two fixes are needed
> to correctly handle them in uprobes.

Could you please add the commit that helps support the insns in the
Changelog.

Rest all looks okay.


Looks good to me.

Acked-by: Srikar Dronamraju <srikar@linux.vnet.ibm.com>

-- 
Thanks and Regards
Srikar Dronamraju

[tip:perf/urgent] uprobes/x86: Fix RIP-relative handling of EVEX-encoded instructions

[tip-bot for Denys Vlasenko]

Commit-ID:  68187872c76a96ed4db7bfb064272591f02e208b
Gitweb:     http://git.kernel.org/tip/68187872c76a96ed4db7bfb064272591f02e208b
Author:     Denys Vlasenko <dvlasenk@redhat.com>
AuthorDate: Thu, 11 Aug 2016 17:45:21 +0200
Committer:  Ingo Molnar <mingo@kernel.org>
CommitDate: Fri, 12 Aug 2016 08:29:24 +0200

uprobes/x86: Fix RIP-relative handling of EVEX-encoded instructions

Since instruction decoder now supports EVEX-encoded instructions, two fixes
are needed to correctly handle them in uprobes.

Extended bits for MODRM.rm field need to be sanitized just like we do it
for VEX3, to avoid encoding wrong register for register-relative access.

EVEX has _two_ extended bits: b and x. Theoretically, EVEX.x should be
ignored by the CPU (since GPRs go only up to 15, not 31), but let's be
paranoid here: proper encoding for register-relative access
should have EVEX.x = 1.

Secondly, we should fetch vex.vvvv for EVEX too.
This is now super easy because instruction decoder populates
vex_prefix.bytes[2] for all flavors of (e)vex encodings, even for VEX2.

Signed-off-by: Denys Vlasenko <dvlasenk@redhat.com>
Acked-by: Masami Hiramatsu <mhiramat@kernel.org>
Acked-by: Srikar Dronamraju <srikar@linux.vnet.ibm.com>
Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Cc: Andy Lutomirski <luto@kernel.org>
Cc: Arnaldo Carvalho de Melo <acme@redhat.com>
Cc: Borislav Petkov <bp@alien8.de>
Cc: Brian Gerst <brgerst@gmail.com>
Cc: H. Peter Anvin <hpa@zytor.com>
Cc: Jim Keniston <jkenisto@us.ibm.com>
Cc: Jiri Olsa <jolsa@redhat.com>
Cc: Josh Poimboeuf <jpoimboe@redhat.com>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
Cc: Oleg Nesterov <oleg@redhat.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Stephane Eranian <eranian@google.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Vince Weaver <vincent.weaver@maine.edu>
Cc: linux-kernel@vger.kernel.org
Cc: <stable@vger.kernel.org> # v4.1+
Fixes: 8a764a875fe3 ("x86/asm/decoder: Create artificial 3rd byte for 2-byte VEX")
Link: http://lkml.kernel.org/r/20160811154521.20469-1-dvlasenk@redhat.com
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 arch/x86/kernel/uprobes.c | 22 +++++++++++-----------
 1 file changed, 11 insertions(+), 11 deletions(-)

diff --git a/arch/x86/kernel/uprobes.c b/arch/x86/kernel/uprobes.c
index 6c1ff31..495c776 100644
--- a/arch/x86/kernel/uprobes.c
+++ b/arch/x86/kernel/uprobes.c
@@ -357,20 +357,22 @@ static void riprel_analyze(struct arch_uprobe *auprobe, struct insn *insn)
 		*cursor &= 0xfe;
 	}
 	/*
-	 * Similar treatment for VEX3 prefix.
-	 * TODO: add XOP/EVEX treatment when insn decoder supports them
+	 * Similar treatment for VEX3/EVEX prefix.
+	 * TODO: add XOP treatment when insn decoder supports them
 	 */
-	if (insn->vex_prefix.nbytes == 3) {
+	if (insn->vex_prefix.nbytes >= 3) {
 		/*
 		 * vex2:     c5    rvvvvLpp   (has no b bit)
 		 * vex3/xop: c4/8f rxbmmmmm wvvvvLpp
 		 * evex:     62    rxbR00mm wvvvv1pp zllBVaaa
-		 *   (evex will need setting of both b and x since
-		 *   in non-sib encoding evex.x is 4th bit of MODRM.rm)
-		 * Setting VEX3.b (setting because it has inverted meaning):
+		 * Setting VEX3.b (setting because it has inverted meaning).
+		 * Setting EVEX.x since (in non-SIB encoding) EVEX.x
+		 * is the 4th bit of MODRM.rm, and needs the same treatment.
+		 * For VEX3-encoded insns, VEX3.x value has no effect in
+		 * non-SIB encoding, the change is superfluous but harmless.
 		 */
 		cursor = auprobe->insn + insn_offset_vex_prefix(insn) + 1;
-		*cursor |= 0x20;
+		*cursor |= 0x60;
 	}
 
 	/*
@@ -415,12 +417,10 @@ static void riprel_analyze(struct arch_uprobe *auprobe, struct insn *insn)
 
 	reg = MODRM_REG(insn);	/* Fetch modrm.reg */
 	reg2 = 0xff;		/* Fetch vex.vvvv */
-	if (insn->vex_prefix.nbytes == 2)
-		reg2 = insn->vex_prefix.bytes[1];
-	else if (insn->vex_prefix.nbytes == 3)
+	if (insn->vex_prefix.nbytes)
 		reg2 = insn->vex_prefix.bytes[2];
 	/*
-	 * TODO: add XOP, EXEV vvvv reading.
+	 * TODO: add XOP vvvv reading.
 	 *
 	 * vex.vvvv field is in bits 6-3, bits are inverted.
 	 * But in 32-bit mode, high-order bit may be ignored.

Re: [PATCH] uprobes/x86: fix rip-relative handling of EVEX-encoded insns.

[Denys Vlasenko]

On 08/12/2016 07:39 AM, Srikar Dronamraju wrote:
> * Denys Vlasenko <dvlasenk@redhat.com> [2016-08-11 17:45:21]:
>
>> Since instruction decoder now supports EVEX-encoded insns, two fixes are needed
>> to correctly handle them in uprobes.
>
> Could you please add the commit that helps support the insns in the
> Changelog.

Sorry, I don't understand the above. Can you rephrase it?