linux-kernel - Re: [PATCHv9 2.6.35-rc4-tip 2/13] uprobes: Breakpoint insertion/removal in user space applications.

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: <20100720072202.GB19375@linux.vnet.ibm.com>
Date:	Tue, 20 Jul 2010 12:52:02 +0530
From:	Srikar Dronamraju <srikar@...ux.vnet.ibm.com>
To:	Christoph Hellwig <hch@...radead.org>
Cc:	Peter Zijlstra <peterz@...radead.org>, Ingo Molnar <mingo@...e.hu>,
	Steven Rostedt <rostedt@...dmis.org>,
	Randy Dunlap <rdunlap@...otime.net>,
	Arnaldo Carvalho de Melo <acme@...radead.org>,
	Linus Torvalds <torvalds@...ux-foundation.org>,
	Masami Hiramatsu <masami.hiramatsu.pt@...achi.com>,
	Ananth N Mavinakayanahalli <ananth@...ibm.com>,
	Oleg Nesterov <oleg@...hat.com>,
	Mark Wielaard <mjw@...hat.com>,
	Mathieu Desnoyers <mathieu.desnoyers@...icios.com>,
	LKML <linux-kernel@...r.kernel.org>,
	Naren A Devaiah <naren.devaiah@...ibm.com>,
	Jim Keniston <jkenisto@...ux.vnet.ibm.com>,
	Frederic Weisbecker <fweisbec@...il.com>,
	"Frank Ch. Eigler" <fche@...hat.com>,
	Andrew Morton <akpm@...ux-foundation.org>,
	"Paul E. McKenney" <paulmck@...ux.vnet.ibm.com>
Subject: Re: [PATCHv9 2.6.35-rc4-tip 2/13]  uprobes: Breakpoint
 insertion/removal in user space applications.

* Christoph Hellwig <hch@...radead.org> [2010-07-20 00:28:14]:

> > +struct user_bkpt_arch_info {
> > +	void (*set_ip)(struct pt_regs *regs, unsigned long vaddr);
> > +	int (*validate_address)(struct task_struct *tsk, unsigned long vaddr);
> > +	int (*read_opcode)(struct task_struct *tsk, unsigned long vaddr,
> > +						user_bkpt_opcode_t *opcode);
> > +	int (*set_bkpt)(struct task_struct *tsk,
> > +				struct user_bkpt *user_bkpt);
> > +	int (*set_orig_insn)(struct task_struct *tsk,
> > +				struct user_bkpt *user_bkpt, bool check);
> > +	bool (*is_bkpt_insn)(struct user_bkpt *user_bkpt);
> > +	int (*analyze_insn)(struct task_struct *tsk,
> > +					struct user_bkpt *user_bkpt);
> > +	int (*pre_xol)(struct task_struct *tsk,
> > +				struct user_bkpt *user_bkpt,
> > +				struct user_bkpt_task_arch_info *tskinfo,
> > +				struct pt_regs *regs);
> > +	int (*post_xol)(struct task_struct *tsk,
> > +				struct user_bkpt *user_bkpt,
> > +				struct user_bkpt_task_arch_info *tskinfo,
> > +				struct pt_regs *regs);
> > +};
> 
> Just wondering why these are function pointers.  Do we exepect an
> architecture to provide different versions of these for say 32 vs 64-bit
> binaries?  If not just making these arch provided helpers might be a lot
> simpler.  Especially in the current version where only very few of these
> are overriden by the architecture at all.
> 
> > +unsigned long uprobes_read_vm(struct task_struct *tsk, void __user *vaddr,
> > +					void *kbuf, unsigned long nbytes)


Some of these functions are purely optional example being
validate_address. 

Some of these functions need not be defined by the architecture in
which case we default to the functions defined in common code. 
examples being: read_opcode, set_bkpt, and set_orig_insn.

Some of these functions are architecture mode specific, for example 
there is a architecture specific pre_xol needed for x86_64. However
generic pre_xol for x86_32 would suffice for x86_32.

Some of these functions need to be mandatorily defined by the
architecture. example being set_ip and analyze_insn.

Apart from the above flexibilities and enforcements that we can make
when we use function pointers, its would be handy to incorporate
more enhancements like return probes and booster. 

> > +{
> > +	if (tsk == current) {
> > +		unsigned long nleft = copy_from_user(kbuf, vaddr, nbytes);
> > +		return nbytes - nleft;
> > +	} else
> > +		return access_process_vm(tsk, (unsigned long) vaddr, kbuf,
> > +							nbytes, 0);
> > +}
> > +
> > +unsigned long uprobes_write_data(struct task_struct *tsk,
> > +				void __user *vaddr, const void *kbuf,
> > +				unsigned long nbytes)
> > +{
> > +	unsigned long nleft;
> > +
> > +	if (tsk == current) {
> > +		nleft = copy_to_user(vaddr, kbuf, nbytes);
> > +		return nbytes - nleft;
> > +	} else
> > +		return access_process_vm(tsk, (unsigned long) vaddr,
> > +						(void *) kbuf, nbytes, 1);
> > +}
> 
> Any reason for the naming mismatch between _read_vm and _write_data?

The naming mismatch was on purpose, we wanted to mention that
write_data cannot be used with code sections unlike read_vm which can
be used to read code section.

> 
> Also I wonder if the optimization for tsk == current should be folded
> directly into access_process_vm instead of adding these wrappers.
> 

One reason why we dont want to move this optimization as is into
access_process_vm is we dont want to do a copy_to_user on a code
section. So that would mean another check to determine if its a code
section before we do the optimization. However there could other
reasons why we shouldnt be doing this optimization. Do you still
think we should still be pursuing with the optimization?

--
Thanks and Regards
Srikar
--
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/