linux-kernel - Re: [PATCH 1/3] x86/alternatives: Teach text_poke

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: <20191003082751.GQ4536@hirez.programming.kicks-ass.net>
Date:   Thu, 3 Oct 2019 10:27:51 +0200
From:   Peter Zijlstra <peterz@...radead.org>
To:     Masami Hiramatsu <mhiramat@...nel.org>
Cc:     linux-kernel@...r.kernel.org, x86@...nel.org,
        Nadav Amit <nadav.amit@...il.com>,
        Andy Lutomirski <luto@...nel.org>,
        Dave Hansen <dave.hansen@...ux.intel.com>,
        Song Liu <songliubraving@...com>,
        Steven Rostedt <rostedt@...dmis.org>,
        Daniel Bristot de Oliveira <bristot@...hat.com>
Subject: Re: [PATCH 1/3] x86/alternatives: Teach text_poke_bp() to emulate
 instructions

On Thu, Oct 03, 2019 at 02:00:50PM +0900, Masami Hiramatsu wrote:

> > This fits almost all text_poke_bp() users, except
> > arch_unoptimize_kprobe() which restores random text, and for that site
> > we have to build an explicit emulate instruction.
> 
> OK, and in this case, I would like to change RELATIVEJUMP_OPCODE
> to JMP32_INSN_OPCODE for readability. (or at least
> making RELATIVEJUMP_OPCODE as an alias of JMP32_INSN_OPCODE)

> > @@ -448,12 +447,18 @@ void arch_optimize_kprobes(struct list_h
> >  void arch_unoptimize_kprobe(struct optimized_kprobe *op)
> >  {
> >  	u8 insn_buff[RELATIVEJUMP_SIZE];
> > +	u8 emulate_buff[RELATIVEJUMP_SIZE];
> >  
> >  	/* Set int3 to first byte for kprobes */
> >  	insn_buff[0] = BREAKPOINT_INSTRUCTION;
> >  	memcpy(insn_buff + 1, op->optinsn.copied_insn, RELATIVE_ADDR_SIZE);
> > +
> > +	emulate_buff[0] = RELATIVEJUMP_OPCODE;
> > +	*(s32 *)(&emulate_buff[1]) = (s32)((long)op->optinsn.insn -
> > +			((long)op->kp.addr + RELATIVEJUMP_SIZE));

I'm halfway through a patch introducing:

  union text_poke_insn {
	u8 code[POKE_MAX_OPCODE_SUZE];
	struct {
		u8 opcode;
		s32 disp;
	} __attribute__((packed));
  };

to text-patching.h to unify all such custom unions we have all over the
place. I'll mob up the above in that.

> > +
> >  	text_poke_bp(op->kp.addr, insn_buff, RELATIVEJUMP_SIZE,
> > -		     op->optinsn.insn);
> > +		     emulate_buff);
> >  }

As argued in a previous thread, text_poke_bp() is broken when it changes
more than a single instruction at a time.

Now, ISTR optimized kprobes does something like:

	poke INT3
	synchronize_rcu_tasks() /* waits for all tasks to schedule
				   guarantees instructions after INT3
				   are unused */
	install optimized probe /* overwrites multiple instrctions with
				   JMP.d32 */

And the above then undoes that by:

	poke INT3 on top of the optimzed probe

	poke tail instructions back /* guaranteed safe because the
				       above INT3 poke ensures the
				       JMP.d32 instruction is unused */

	poke head byte back

Is this correct? If so, we should probably put a comment in there
explaining how all this is unusual but safe.