Messages in this thread | | | Date | Sun, 24 Mar 2013 16:26:51 +0100 | From | Oleg Nesterov <> | Subject | Re: [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.
| |