linux-kernel - Re: [PATCH 5/7] uretprobes: return probe exit, invoke handlers

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20130324162817.GD17037@redhat.com>
Date:	Sun, 24 Mar 2013 17:28:17 +0100
From:	Oleg Nesterov <oleg@...hat.com>
To:	Anton Arapov <anton@...hat.com>
Cc:	Srikar Dronamraju <srikar@...ux.vnet.ibm.com>,
	LKML <linux-kernel@...r.kernel.org>,
	Josh Stone <jistone@...hat.com>,
	Frank Eigler <fche@...hat.com>,
	Peter Zijlstra <peterz@...radead.org>,
	Ingo Molnar <mingo@...e.hu>,
	Ananth N Mavinakayanahalli <ananth@...ibm.com>,
	adrian.m.negreanu@...el.com, Torsten.Polle@....de
Subject: Re: [PATCH 5/7] uretprobes: return probe exit, invoke handlers

On 03/22, Anton Arapov wrote:
>
> +static void handle_uretprobe(struct xol_area *area, struct pt_regs *regs)
> +{
> +	struct uprobe_task *utask;
> +	struct return_instance *ri, *tmp;
> +	unsigned long prev_ret_vaddr;
> +
> +	utask = get_utask();
> +	if (!utask)
> +		return;
> +
> +	ri = utask->return_instances;
> +	if (!ri)
> +		return;

Hmm. I am wondering what should the caller (handle_swbp) do in this
case...

> +
> +	instruction_pointer_set(regs, ri->orig_ret_vaddr);
> +
> +	while (ri) {
> +		if (ri->uprobe->consumers)
> +			handler_uretprobe_chain(ri->uprobe, regs);

I'd suggest to either remove this check or move it into
handler_uretprobe_chain().

> +
> +		put_uprobe(ri->uprobe);
> +		tmp = ri;
> +		prev_ret_vaddr = tmp->orig_ret_vaddr;

For what? It seems that prev_ret_vaddr should be simply killed.

> +		ri = ri->next;
> +		kfree(tmp);

Another case when you do put_uprobe/kfree using the different vars...
Once again, the code is correct but imho a bit confusing.

> +		if (!ri || ri->dirty == false) {
> +			/*
> +			 * This is the first return uprobe (chronologically)
> +			 * pushed for this particular instance of the probed
> +			 * function.
> +			 */
> +			utask->return_instances = ri;
> +			return;
> +		}

Else? we simply return without updating ->return_instances which
points to the freed element(s) ? OK, this must not be possible but
this is not obvious...

And the fact you check "ri != NULL" twice doesn't look very nice.
We already checked ri != NULL before while(ri), we have to do this
anyway for instruction_pointer_set(). Perhaps do/whild or even
for (;;) + break would be more clean. But this is minor.

I am not sure the logic is correct. OK. suppose that
->return_instances = NULL.

The task hits the rp breakoint. After that

	return_instances -> { .dirty = false }

The task hits the same breakoint before return (tail call), now
we have

	return_instances -> { .dirty = true } -> { .dirty = false }

Then it returns and handle_uretprobe() should unwind the whole stack.
But, it seems, the main loop will stop after the 1st iteration?

Ignoring the fact you need put_uprobe/kfree, it seems that we should
do something like this,

	do {
		handler_uretprobe_chain(...);

		if (!ri->dirty)	// not chained
			break;

		ri = ri->next;		
	} while (ri);

	utask->return_instances = ri;

No?

> @@ -1631,11 +1681,19 @@ static void handle_swbp(struct pt_regs *regs)
>  {
>  	struct uprobe *uprobe;
>  	unsigned long bp_vaddr;
> +	struct xol_area *area;
>  	int uninitialized_var(is_swbp);
>  
>  	bp_vaddr = uprobe_get_swbp_addr(regs);
> -	uprobe = find_active_uprobe(bp_vaddr, &is_swbp);
> +	area = get_xol_area();

Why?

No, we do not want this heavy and potentially unnecessary get_xol_area(),

> +	if (area) {

Just check uprobes_state.xol_area != NULL instead. If it is NULL
we simply should not call handle_uretprobe().

Or perhaps get_trampoline_vaddr() should simply return -1 if
->xol_area == NULL.

> +		if (bp_vaddr == get_trampoline_vaddr(area)) {

I just noticed get_trampoline_vaddr() takes an argument... It should
not, I think.

Oleg.

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