linux-kernel - Re: [PATCH v2] uprobes: simplify rip-relative handling

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] [day] [month] [year] [list]

Message-ID: <20140428153143.GA20901@redhat.com>
Date:	Mon, 28 Apr 2014 17:31:43 +0200
From:	Oleg Nesterov <oleg@...hat.com>
To:	Denys Vlasenko <dvlasenk@...hat.com>
Cc:	linux-kernel@...r.kernel.org, Jim Keniston <jkenisto@...ibm.com>,
	Masami Hiramatsu <masami.hiramatsu.pt@...achi.com>
Subject: Re: [PATCH v2] uprobes: simplify rip-relative handling

On 04/28, Denys Vlasenko wrote:
>
> It is possible to replace rip-relative addressing mode
> with addressing mode of the same length: (reg+disp32).
> This eliminates the need to fix up immediate
> and instruction length.

"and change instruction length", I presume.

I like this patch very much, and I would be happy to ack everything
except this part:

> @@ -307,22 +313,13 @@ static void riprel_analyze(struct arch_uprobe *auprobe, struct insn *insn)
>  		 * #1) for the scratch register.
>  		 */
>  		auprobe->def.fixups |= UPROBE_FIX_RIP_CX;
> -		/* Change modrm from 00 000 101 to 00 000 001. */
> -		*cursor = 0x1;
> +		/* Change modrm from 00 000 101 to 10 000 001. */
> +		*cursor = 0x81;
>  	} else {
>  		/* Use %rax (register #0) for the scratch register. */
>  		auprobe->def.fixups |= UPROBE_FIX_RIP_AX;
> -		/* Change modrm from 00 xxx 101 to 00 xxx 000 */
> -		*cursor = (reg << 3);
> -	}
> -
> -	/* Target address = address of next instruction + (signed) offset */
> -	auprobe->def.riprel_target = (long)insn->length + insn->displacement.value;
> -
> -	/* Displacement field is gone; slide immediate field (if any) over. */
> -	if (insn->immediate.nbytes) {
> -		cursor++;
> -		memmove(cursor, cursor + insn->displacement.nbytes, insn->immediate.nbytes);
> +		/* Change modrm from 00 xxx 101 to 10 xxx 000 */
> +		*cursor = (reg << 3) | 0x80;
>  	}

As you know, this is black magic for me.

Masami, Jim, please review and ack/nack?

> @@ -343,26 +340,17 @@ static void riprel_pre_xol(struct arch_uprobe *auprobe, struct pt_regs *regs)
>  		unsigned long *sr = scratch_reg(auprobe, regs);
>  
>  		utask->autask.saved_scratch_register = *sr;
> -		*sr = utask->vaddr + auprobe->def.riprel_target;
> +		*sr = utask->vaddr + (int)auprobe->def.ilen;
                                     ^^^^^

Why? This is pointless and even misleading. I'll remove this part if you
do not object.

Oleg.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/