lkml.org 
[lkml]   [2013]   [Mar]   [24]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH 4/7] uretprobes: return probe entry, prepare_uretprobe()
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;

Oleg.



\
 
 \ /
  Last update: 2013-03-24 16:41    [W:0.190 / U:0.700 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site