linux-kernel - Re: [PATCH] uprobes/x86: emulate push insns for uprobe on x86

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: <20171110130611.GA20527@redhat.com>
Date:   Fri, 10 Nov 2017 14:06:11 +0100
From:   Oleg Nesterov <oleg@...hat.com>
To:     Yonghong Song <yhs@...com>
Cc:     mingo@...nel.org, tglx@...utronix.de, peterz@...radead.org,
        linux-kernel@...r.kernel.org, x86@...nel.org,
        netdev@...r.kernel.org, ast@...com, kernel-team@...com
Subject: Re: [PATCH] uprobes/x86: emulate push insns for uprobe on x86

Yonghong,

The patch looks good to me, but I'll try to read it carefully later.
Just a couple of cosmetic nits for now.

On 11/09, Yonghong Song wrote:
>
> --- a/arch/x86/include/asm/uprobes.h
> +++ b/arch/x86/include/asm/uprobes.h
> @@ -53,6 +53,10 @@ struct arch_uprobe {
>  			u8	fixups;
>  			u8	ilen;
>  		} 			defparam;
> +		struct {
> +			u8	src_offset;	/* to the start of pt_regs */
> +			u8	ilen;
> +		}			push;
>  	};
>  };

I know it is very easy to blame the naming ;) but src_offset doesn't
look nice. How about reg_offset ?

> +static bool push_emulate_op(struct arch_uprobe *auprobe, struct pt_regs *regs)
> +{
> +	void *src_ptr = (void *)regs + auprobe->push.src_offset;
> +
> +	if (emulate_push_stack(regs, *(unsigned long *)src_ptr))
> +		return false;

You can declare src_ptr as

	unsigned long *src_ptr = ...;

and avoid the casting below.

> +static int uprobe_setup_xol_ops(struct arch_uprobe *auprobe, struct insn *insn)
>  {
>  	u8 opc1 = OPCODE1(insn);
>  	int i;
>
>  	switch (opc1) {
> +	case 0x50 ... 0x57:
> +		return uprobe_setup_push_ops(auprobe, insn, opc1);
> +
>  	case 0xeb:	/* jmp 8 */
>  	case 0xe9:	/* jmp 32 */
>  	case 0x90:	/* prefix* + nop; same as jmp with .offs = 0 */
> @@ -767,7 +863,7 @@ int arch_uprobe_analyze_insn(struct arch_uprobe *auprobe, struct mm_struct *mm,
>  	if (ret)
>  		return ret;
>
> -	ret = branch_setup_xol_ops(auprobe, &insn);
> +	ret = uprobe_setup_xol_ops(auprobe, &insn);
>  	if (ret != -ENOSYS)
>  		return ret;

Well... again, this is cosmetic and I won't insist, but to me it would be
more clean to not change/rename branch_setup_xol_ops(). Instead, you can
just add

	ret = uprobe_setup_push_ops(...);
	if (ret != -ENOSYS)
		return ret;

at the start of arch_uprobe_analyze_insn(), right after the similar
branch_setup_xol_ops() call.

Btw... please add v2 into the subject when you send the new version.

Oleg.