linux-kernel - Re: [PATCH v4 13/16] ARM: Add an emulate flag to the kprobes/uprobes instruction decode functions

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:	Thu, 16 Jan 2014 09:18:52 +0000
From:	"Jon Medhurst (Tixy)" <tixy@...aro.org>
To:	David Long <dave.long@...aro.org>
Cc:	linux-arm-kernel@...ts.infradead.org,
	Russell King <linux@....linux.org.uk>,
	Rabin Vincent <rabin@....in>, Oleg Nesterov <oleg@...hat.com>,
	Srikar Dronamraju <srikar@...ux.vnet.ibm.com>,
	Ingo Molnar <mingo@...hat.com>,
	Masami Hiramatsu <masami.hiramatsu.pt@...achi.com>,
	Ananth N Mavinakayanahalli <ananth@...ibm.com>,
	Anil S Keshavamurthy <anil.s.keshavamurthy@...el.com>,
	davem@...emloft.net, Peter Zijlstra <a.p.zijlstra@...llo.nl>,
	Paul Mackerras <paulus@...ba.org>,
	Arnaldo Carvalho de Melo <acme@...stprotocols.net>,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH v4 13/16] ARM: Add an emulate flag to the
 kprobes/uprobes instruction decode functions

On Wed, 2014-01-15 at 14:31 -0500, David Long wrote:
> On 12/20/13 09:58, Jon Medhurst (Tixy) wrote:
> > On Sun, 2013-12-15 at 23:08 -0500, David Long wrote:
[...]
> >>   {
> >>   #ifdef CONFIG_THUMB2_KERNEL
> >>   	if (thumb) {
> >> @@ -253,7 +253,7 @@ set_emulated_insn(probes_opcode_t insn, struct arch_specific_insn *asi,
> >>    * non-zero value, the corresponding nibble in pinsn is validated and modified
> >>    * according to the type.
> >>    */
> >> -static bool __kprobes decode_regs(probes_opcode_t *pinsn, u32 regs)
> >> +static bool __kprobes decode_regs(probes_opcode_t *pinsn, u32 regs, bool modify)
> >>   {
> >>   	probes_opcode_t insn = *pinsn;
> >>   	probes_opcode_t mask = 0xf; /* Start at least significant nibble */
> >> @@ -317,9 +317,16 @@ static bool __kprobes decode_regs(probes_opcode_t *pinsn, u32 regs)
> >>   		/* Replace value of nibble with new register number... */
> >>   		insn &= ~mask;
> >>   		insn |= new_bits & mask;
> >> +		if (modify) {
> >> +			/* Replace value of nibble with new register number */
> >> +			insn &= ~mask;
> >> +			insn |= new_bits & mask;
> >> +		}
> >
> > Huh? As is, the above addition doesn't do anything because insn has
> > already been modified. I guess you played with the idea that you needed
> > to avoid changing insn (you don't) and then didn't undo the experiment
> > quite right. :-)
> >
> 
> The conditional modification of the instruction was part of Rabin's 
> original work for uprobes, but I messed up the merge from an earlier 
> working version of my patches.  My intention was/is to delete the old 
> unconditional code.  Sounds like maybe you disagree though.  The intent 
> is to only modify the instruction in the kprobes case.

'insn' is the local variable containing the instruction value we're
processing. It doesn't matter if we change that, we just need to avoid
updating the instruction in memory, which the code in the next chunk
already correctly checks for...

> >>   	}
> >>
> >> -	*pinsn = insn;
> >> +	if (modify)
> >> +		*pinsn = insn;
> >> +
> >>   	return true;
> >>

So only one of these 'if (modify)' checks is required for code
correctness, and I suggest keeping the second one as it's more explicit
and defensive.


-- 
Tixy

--
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/