linux-kernel - Re: [PATCH v3 1/6] 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]

Date:   Tue, 8 Oct 2019 16:48:34 +0200
From:   Peter Zijlstra <peterz@...radead.org>
To:     Borislav Petkov <bp@...en8.de>
Cc:     x86@...nel.org, linux-kernel@...r.kernel.org, rostedt@...dmis.org,
        mhiramat@...nel.org, bristot@...hat.com, jbaron@...mai.com,
        torvalds@...ux-foundation.org, tglx@...utronix.de,
        mingo@...nel.org, namit@...are.com, hpa@...or.com, luto@...nel.org,
        ard.biesheuvel@...aro.org, jpoimboe@...hat.com
Subject: Re: [PATCH v3 1/6] x86/alternatives: Teach text_poke_bp() to emulate
 instructions

On Tue, Oct 08, 2019 at 04:29:24PM +0200, Borislav Petkov wrote:
> On Mon, Oct 07, 2019 at 10:17:17AM +0200, Peter Zijlstra wrote:

> > @@ -63,8 +66,17 @@ static inline void int3_emulate_jmp(stru
> >  	regs->ip = ip;
> >  }
> >  
> > -#define INT3_INSN_SIZE 1
> > -#define CALL_INSN_SIZE 5
> > +#define INT3_INSN_SIZE		1
> > +#define INT3_INSN_OPCODE	0xCC
> > +
> > +#define CALL_INSN_SIZE		5
> > +#define CALL_INSN_OPCODE	0xE8
> > +
> > +#define JMP32_INSN_SIZE		5
> > +#define JMP32_INSN_OPCODE	0xE9
> > +
> > +#define JMP8_INSN_SIZE		2
> > +#define JMP8_INSN_OPCODE	0xEB
> 
> You probably should switch those to have the name prefix come first and
> make them even shorter:
> 
> OPCODE_CALL
> INSN_SIZE_CALL
> OPCODE_JMP32
> INSN_SIZE_JMP32
> OPCODE_JMP8
> ...
> 
> This way you have the opcodes prefixed with OPCODE_ and the insn sizes
> with INSN_SIZE_. I.e., what they actually are.

I really don't like that; the important part is which instruction and
that really should come first. Also, your variant is horribly
inconsistent.

> > --- a/arch/x86/kernel/alternative.c
> > +++ b/arch/x86/kernel/alternative.c
> 
> ...
> 
> > @@ -1027,9 +1046,9 @@ NOKPROBE_SYMBOL(poke_int3_handler);
> >   */
> >  void text_poke_bp_batch(struct text_poke_loc *tp, unsigned int nr_entries)
> >  {
> > -	int patched_all_but_first = 0;
> > -	unsigned char int3 = 0xcc;
> > +	unsigned char int3 = INT3_INSN_OPCODE;
> >  	unsigned int i;
> > +	int do_sync;
> >  
> >  	lockdep_assert_held(&text_mutex);
> >  
> > @@ -1053,16 +1072,16 @@ void text_poke_bp_batch(struct text_poke
> >  	/*
> >  	 * Second step: update all but the first byte of the patched range.
> >  	 */
> > -	for (i = 0; i < nr_entries; i++) {
> > +	for (do_sync = 0, i = 0; i < nr_entries; i++) {
> >  		if (tp[i].len - sizeof(int3) > 0) {
> >  			text_poke((char *)tp[i].addr + sizeof(int3),
> > -				  (const char *)tp[i].opcode + sizeof(int3),
> > +				  (const char *)tp[i].text + sizeof(int3),
> >  				  tp[i].len - sizeof(int3));
> > -			patched_all_but_first++;
> > +			do_sync++;
> >  		}
> >  	}
> >  
> > -	if (patched_all_but_first) {
> > +	if (do_sync) {
> >  		/*
> >  		 * According to Intel, this core syncing is very likely
> >  		 * not necessary and we'd be safe even without it. But
> > @@ -1075,10 +1094,17 @@ void text_poke_bp_batch(struct text_poke
> >  	 * Third step: replace the first byte (int3) by the first byte of
> >  	 * replacing opcode.
> >  	 */
> > -	for (i = 0; i < nr_entries; i++)
> > -		text_poke(tp[i].addr, tp[i].opcode, sizeof(int3));
> > +	for (do_sync = 0, i = 0; i < nr_entries; i++) {
> 
> Can we have the do_sync reset outside of the loop?

Can, but why? That's more lines for no raisin ;-)

> > +		if (tp[i].text[0] == INT3_INSN_OPCODE)
> > +			continue;
> 
> I'm guessing we preset the 0th byte to 0xcc somewhere.... I just can't
> seem to find it...

Very first pass, we write INT3's everywhere.