lkml.org 
[lkml]   [2020]   [Mar]   [7]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH] kretprobe: check re-registration of the same kretprobe earlier
On Sat, 7 Mar 2020 10:16:35 +0800
"chengjian (D)" <cj.chengjian@huawei.com> 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@huawei.com> 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@kernel.org>
> >
> > 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@kernel.org>

\
 
 \ /
  Last update: 2020-03-07 10:55    [W:0.048 / U:5.712 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site