Re: [PATCH] uprobes: Free utask on dup_return_instance() failure

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [Critical] Double free of `extra_consumers` during `fork()` due to missing pointer nullification in `dup_return_instance()` / `ri_pool_push()`.
--

--- Patch [1]: [PATCH] uprobes: Free utask on dup_return_instance() failure ---
commit 52bd4caeb5b9686b25fe707609034e9397c6100a
Author: Keke Ming <[email protected]>

uprobes: Free utask on dup_return_instance() failure

dup_utask() installs the new uprobe_task in t->utask before copying
return_instances. If dup_return_instance() fails, the partially copied
utask is left attached to the child task. This patch frees the partially
copied utask before returning -ENOMEM.

> diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
> index 7709ea8824778..7547929ab294d 100644
> --- a/kernel/events/uprobes.c
> +++ b/kernel/events/uprobes.c
> @@ -2140,8 +2140,10 @@ static int dup_utask(struct task_struct *t, struct uprobe_task *o_utask)
>  	p = &n_utask->return_instances;
>  	for (o = o_utask->return_instances; o; o = o->next) {
>  		n = dup_return_instance(o);
> -		if (!n)
> +		if (!n) {
> +			uprobe_free_utask(t);

[Severity: Critical]
This is a pre-existing issue, but can dup_return_instance() leave a dangling
pointer that results in a double free of extra_consumers when
uprobe_free_utask() is called here?

If a multi-consumer uretprobe is hit, extra_consumers is allocated. When this
returns, it is pushed to the reuse pool:

kernel/events/uprobes.c:ri_pool_push() {
    ri->cons_cnt = 0;
    ri->next = utask->ri_pool;
    utask->ri_pool = ri;
}

This resets cons_cnt to 0 but retains the extra_consumers heap pointer.

If the thread then hits a single-consumer uretprobe, alloc_return_instance()
reuses the pooled object. push_consumer() increments cons_cnt to 1 and uses
the inline consumer, leaving the old extra_consumers pointer untouched.

When the task calls fork(), dup_return_instance() does a shallow copy:

kernel/events/uprobes.c:dup_return_instance() {
    ri = kmemdup(old, sizeof(*ri), GFP_KERNEL);
    if (!ri)
        return NULL;

    if (unlikely(old->cons_cnt > 1)) {
        ri->extra_consumers = kmemdup(old->extra_consumers, ...
    ...
}

Since old->cons_cnt == 1, it skips deep-copying or clearing extra_consumers.
Both the parent and child tasks now share the exact same heap pointer. 

When the tasks exit, or if dup_return_instance() fails and triggers the new
cleanup path in this patch, ri_free() will execute kfree(ri->extra_consumers)
in both contexts, freeing the same memory twice.

>  			return -ENOMEM;
> +		}
>  
>  		/* if uprobe is non-NULL, we'll have an extra refcount for uprobe */
>  		uprobe = hprobe_expire(&o->hprobe, true);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1