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

[Oleg Nesterov <oleg@redhat.com>]

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
>