Messages in this thread | | | Date | Sat, 7 Mar 2020 18:54:39 +0900 | From | Masami Hiramatsu <> | Subject | Re: [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>
| |