Re: [PATCH 4/7] uretprobes: return probe entry, prepare_uretprobe()

[Anton Arapov]

On Sun, Mar 24, 2013 at 04:26:51PM +0100, Oleg Nesterov wrote:
> On 03/22, Anton Arapov wrote:
> >
> >  void uprobe_free_utask(struct task_struct *t)
> >  {
> >  	struct uprobe_task *utask = t->utask;
> > +	struct return_instance *ri, *tmp;
> >
> >  	if (!utask)
> >  		return;
> > @@ -1325,6 +1334,15 @@ void uprobe_free_utask(struct task_struct *t)
> >  	if (utask->active_uprobe)
> >  		put_uprobe(utask->active_uprobe);
> >
> > +	ri = utask->return_instances;
> 
> You also need to nullify ->return_instances before return, otherwise
> it can be use-after-freed later.
> 
> uprobe_free_utask() can also be called when the task execs.
> 
> > +	while (ri) {
> > +		put_uprobe(ri->uprobe);
> > +
> > +		tmp = ri;
> > +		ri = ri->next;
> > +		kfree(tmp);
> > +	}
> 
> This is really minor, but I can't resist. Both put_uprobe() and kfree()
> work with the same object, it would be more clean to use the same var.
> Say,
> 
> 	while (ri) {
> 		tmp = ri;
> 		ri = ri->next;
> 
> 		put_uprobe(tmp->uprobe);
> 		kfree(tmp);
> 	}
> 
> > +static void prepare_uretprobe(struct uprobe *uprobe, struct pt_regs *regs)
> > +{
> ...
> > +
> > +	prev_ret_vaddr = -1;
> > +	if (utask->return_instances)
> > +		prev_ret_vaddr = utask->return_instances->orig_ret_vaddr;
> > +
> > +	ri = kzalloc(sizeof(struct return_instance), GFP_KERNEL);
> > +	if (!ri)
> > +		return;
> > +
> > +	ri->dirty = false;
> > +	trampoline_vaddr = get_trampoline_vaddr(area);
> > +	ret_vaddr = arch_uretprobe_hijack_return_addr(trampoline_vaddr, regs);
> > +
> > +	/*
> > +	 * We don't want to keep trampoline address in stack, rather keep the
> > +	 * original return address of first caller thru all the consequent
> > +	 * instances. This also makes breakpoint unwrapping easier.
> > +	 */
> > +	if (ret_vaddr == trampoline_vaddr) {
> > +		if (likely(prev_ret_vaddr != -1)) {
> > +			ri->dirty = true;
> > +			ret_vaddr = prev_ret_vaddr;
> > +		} else {
> > +			/*
> > +			 * This situation is not possible. Likely we have an
> > +			 * attack from user-space. Die.
> > +			 */
> > +			printk(KERN_ERR "uprobe: something went wrong "
> > +				"pid/tgid=%d/%d", current->pid, current->tgid);
> > +			send_sig(SIGSEGV, current, 0);
> > +			kfree(ri);
> > +			return;
> > +		}
> > +	}
> > +
> > +	if (likely(ret_vaddr != -1)) {
> > +		atomic_inc(&uprobe->ref);
> > +		ri->uprobe = uprobe;
> > +		ri->orig_ret_vaddr = ret_vaddr;
> > +
> > +		/* add instance to the stack */
> > +		ri->next = utask->return_instances;
> > +		utask->return_instances = ri;
> > +
> > +		return;
> > +	}
> > +
> > +	kfree(ri);
> > +}
> 
> Anton, this really doesn't look clear/clean. Why do you need prev_ret_vaddr
> in advance? Why do you need it at all? why do you delay the "ret_vaddr == -1"
> errorcheck?
> 
> And ->dirty looks confusing... perhaps ->chained ?
> 
> 		ri = kzalloc(...);
> 		if (!ri)
> 			return;
> 
> 		ret_vaddr = arch_uretprobe_hijack_return_addr(...);
> 		if (ret_vaddr == -1)
> 			goto err;
> 
> 		if (ret_vaddr == trampoline_vaddr) {
> 			if (!utask->return_instances) {
> 				// This situation is not possible.
> 				// (not sure we should send SIGSEGV)
> 				pr_warn(...);
> 				goto err;
> 			}
> 
> 			ri->chained = true;
> 			ret_vaddr = utask->return_instances->orig_ret_vaddr;
> 		}
> 
> 		fill-ri-and-add-push-it;
> 		return;
> 
> 	err:
> 		kfree(ri);
> 		return;


I will do the appropriate changes. Thanks!

Anton.

> Oleg.
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/