Re: [PATCH] kretprobe: check re-registration of the same kretprobe earlier

[Masami Hiramatsu]

On Sat, 7 Mar 2020 10:16:35 +0800
"chengjian (D)" <cj.chengjian@xxxxxxxxxx> wrote:

> 
> On 2020/3/6 23:21, Masami Hiramatsu wrote:
> > Hi Cheng,
> >
> > On Fri, 6 Mar 2020 17:35:06 +0800
> > Cheng Jian <cj.chengjian@xxxxxxxxxx> wrote:
> >
> >> Our system encountered a use-after-free when re-register a
> >> same kretprobe. it access the hlist node in rp->free_instances
> >> which has been released already.
> >>
> >> Prevent re-registration has been implemented for kprobe before,
> >> but it's too late for kretprobe. We must check the re-registration
> >> before re-initializing the kretprobe, otherwise it will destroy the
> >> data and struct of the kretprobe registered, it can lead to memory
> >> leak and use-after-free.
> > I couldn't get how it cause use-after-free, but it causes memory leak.
> > Anyway, if we can help to find a wrong usage, it might be good.
> >
> > Acked-by: Masami Hiramatsu <mhiramat@xxxxxxxxxx>
> >
> > BTW, I think we should use WARN_ON() for this error, because this
> > means the caller is completely buggy.
> >
> > Thank you,
> 
> Hi Masami
> 
> When we try to re-register the same kretprobe, the register_kprobe() 
> return failed and try to free_rp_inst
> 
>  ÂÂÂ register_kretprobe()
> 
>  ÂÂÂ ÂÂÂ raw_spin_lock_init(&rp->lock);
>  ÂÂ Â ÂÂ INIT_HLIST_HEAD(&rp->free_instances);ÂÂÂ # re-init
> 
>  ÂÂÂÂÂÂÂ inst = kmalloc(sizeof(struct kretprobe_instance) + 
> p->data_size, GFP_KERNEL); # re-alloc
> 
>  ÂÂÂÂÂÂÂ ret = register_kprobe(&rp->kp);Â # failed
> 
>  ÂÂÂÂÂÂÂ free_rp_inst(rp);ÂÂ # free all the free_instances
> 
> at the same time, the kretprobe registed handle(trigger), it tries to 
> access the free_instances.
> 
> Since we broke the rp->lock and free_instances when re-registering, we 
> are accessing the newly
> 
> allocated free_instances and it's being released.
> 
> pre_handler_kretprobe
> 
>  ÂÂÂ ri = hlist_entry(rp->free_instances.first, struct 
> kretprobe_instance, hlist); # access the new free_instances. BOOM!!!
> 
>  Â Â hlist_del(&ri->hlist); #BOOM!!!

Ah, I see. I thought that you said ri is use-after-free, but in reality,
rp is use-after-free (use-after-init). OK.

> And the problem here is destructive, it destroyed all the data of the 
> previously registered kretprobe,
> it can lead to a system crash, memory leak, use-after-free and even some 
> other unexpected behavior.

Yes, so I think we should do 

+	/* Return error if it's being re-registered */
+	ret = check_kprobe_rereg(&rp->kp);
+	if (WARN_ON(ret))
+		return ret;

This will give a warning message to the developer.

Thank you,

-- 
Masami Hiramatsu <mhiramat@xxxxxxxxxx>