[PATCH] kernel/kprobes: add check to avoid kprobe memory leak

[PATCH] kernel/kprobes: add check to avoid kprobe memory leak

[JianKang Chen]

The function register_kretprobe is used to initialize a struct
kretprobe and allocate a list table for kprobe instance.
However,in this function, there is a memory leak.

The test case:

static struct kretprobe rp;
struct  kretprobe *rps[10]={&rp ,&rp ,&rp ,
&rp ,&rp ,&rp ,&rp ,&rp ,&rp,&rp};

static int ret_handler(struct kretprobe_instance *ri, struct pt_regs
*regs)
{
        printk(KERN_DEBUG "ret_handler\n");
        return 0;
}
static int entry_handler(struct kretprobe_instance *ri, struct pt_regs
*regs)
{
        printk(KERN_DEBUG "entry_handler\n");
        return 0;
}
static int __init kretprobe_init(void)
{
        int ret;
        rp.kp.addr = (kprobe_opcode_t *)kallsyms_lookup_name("do_fork");
        rp.handler=ret_handler;
        rp.entry_handler=entry_handler;
        rp.maxactive = 1;

        ret = register_kretprobes(rps,10);
        if (ret < 0)
        {
                return -1;
        }

Result:

unreferenced object 0xffff8012aea35500 (size 64):
  comm "insmod", pid 31966, jiffies 4309153380 (age 111.356s)
  hex dump (first 32 bytes):
    00 00 00 00 00 00 00 00 00 85 37 fc ff 7f ff ff  ..........7.....
    08 55 a3 ae 12 80 ff ff 01 00 00 00 00 00 00 00  .U..............
  backtrace:
    [<ffff8000002cd880>] create_object+0x1e0/0x3f0
    [<ffff800000fa47ac>] kmemleak_alloc+0x6c/0xf0
    [<ffff8000002ac97c>] __kmalloc+0x23c/0x2e0
    [<ffff8000001a6f2c>] register_kretprobe+0x12c/0x350
    [<ffff8000001a7198>] register_kretprobes+0x48/0xa0
    [<ffff7ffffc380058>] 0xffff7ffffc380058
    [<ffff800000084be0>] do_one_initcall+0x120/0x260
    [<ffff800000faa1a4>] do_init_module+0xf0/0x288
    [<ffff8000001736f4>] load_module+0x1824/0x1b50
    [<ffff800000173cc8>] SyS_finit_module+0xc8/0x100
    [<ffff800000084778>] __sys_trace_return+0x0/0x4
    [<ffffffffffffffff>] 0xffffffffffffffff

when there are two same struct kretprobe rp,
the INIT_HLIST_HEAD() will result
in a empty list table rp->free_instances.
The memory leak will happen.
So it needs to check the list table before INIT_HLIST_HEAD().

Reported-and-tested-by: Wen Kang <kangwen1@huawei.com>
Signed-off-by: JianKang Chen <chenjiankang1@huawei.com>
Signed-off-by: xie yisheng <xieyisheng1@huawei.com>
Signed-off-by: Bixuan Cui <cuibixuan@huawei.com>
---
 kernel/kprobes.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/kernel/kprobes.c b/kernel/kprobes.c
index a1606a4..8b3330c 100644
--- a/kernel/kprobes.c
+++ b/kernel/kprobes.c
@@ -1948,6 +1948,10 @@ int register_kretprobe(struct kretprobe *rp)
 #endif
 	}
 	raw_spin_lock_init(&rp->lock);
+
+	if (!hlist_empty(&rp->free_instances))
+		return -EBUSY;
+
 	INIT_HLIST_HEAD(&rp->free_instances);
 	for (i = 0; i < rp->maxactive; i++) {
 		inst = kmalloc(sizeof(struct kretprobe_instance) +
-- 
1.7.12.4

Re: [PATCH] kernel/kprobes: add check to avoid kprobe memory leak

[Masami Hiramatsu]

On Tue, 24 Oct 2017 20:17:02 +0800
JianKang Chen <chenjiankang1@huawei.com> wrote:

> The function register_kretprobe is used to initialize a struct
> kretprobe and allocate a list table for kprobe instance.
> However,in this function, there is a memory leak.
> 
> The test case:
> 
> static struct kretprobe rp;
> struct  kretprobe *rps[10]={&rp ,&rp ,&rp ,
> &rp ,&rp ,&rp ,&rp ,&rp ,&rp,&rp};

What ? this is buggy code. you must not list same kretprobe.
But, year, since register_kprobe() already has similar protection against
reusing, register_kretprobe() should do so.

[..]
>  	raw_spin_lock_init(&rp->lock);
> +
> +	if (!hlist_empty(&rp->free_instances))
> +		return -EBUSY;
> +

Hmm, but can you use check_kprobe_rereg() before raw_spin_lock_init()?
If user reuses rp after it starts, rp->lock can already be used.

Thank you,

>  	INIT_HLIST_HEAD(&rp->free_instances);
>  	for (i = 0; i < rp->maxactive; i++) {
>  		inst = kmalloc(sizeof(struct kretprobe_instance) +
> -- 
> 1.7.12.4
> 


-- 
Masami Hiramatsu <mhiramat@kernel.org>

Re: [PATCH] kernel/kprobes: add check to avoid kprobe memory leak

[chenjiankang]

> On Tue, 24 Oct 2017 20:17:02 +0800
> JianKang Chen <chenjiankang1@huawei.com> wrote:
> 
>> The function register_kretprobe is used to initialize a struct
>> kretprobe and allocate a list table for kprobe instance.
>> However,in this function, there is a memory leak.
>>
>> The test case:
>>
>> static struct kretprobe rp;
>> struct  kretprobe *rps[10]={&rp ,&rp ,&rp ,
>> &rp ,&rp ,&rp ,&rp ,&rp ,&rp,&rp};
> 
> What ? this is buggy code. you must not list same kretprobe.
> But, year, since register_kprobe() already has similar protection against
> reusing, register_kretprobe() should do so.
> 
> [..]
>>  	raw_spin_lock_init(&rp->lock);
>> +
>> +	if (!hlist_empty(&rp->free_instances))
>> +		return -EBUSY;
>> +
> 
> Hmm, but can you use check_kprobe_rereg() before raw_spin_lock_init()?
> If user reuses rp after it starts, rp->lock can already be used.

Hmm, your advice is very good, we can use check_kprobe_rereg() at 
the beginning of the register_kretprobe();

For example:

int register_kretprobe(struct kretprobe *rp)
{
        int ret = 0;
        struct kretprobe_instance *inst;
        int i;
        void *addr;

        ret = check_kprobe_rereg(&rp->kp);
        if (ret)
                return ret;

Thank you!
 

Re: [PATCH] kernel/kprobes: add check to avoid kprobe memory leak

[Masami Hiramatsu]

On Thu, 26 Oct 2017 09:22:23 +0800
chenjiankang <chenjiankang1@huawei.com> wrote:

> > On Tue, 24 Oct 2017 20:17:02 +0800
> > JianKang Chen <chenjiankang1@huawei.com> wrote:
> > 
> >> The function register_kretprobe is used to initialize a struct
> >> kretprobe and allocate a list table for kprobe instance.
> >> However,in this function, there is a memory leak.
> >>
> >> The test case:
> >>
> >> static struct kretprobe rp;
> >> struct  kretprobe *rps[10]={&rp ,&rp ,&rp ,
> >> &rp ,&rp ,&rp ,&rp ,&rp ,&rp,&rp};
> > 
> > What ? this is buggy code. you must not list same kretprobe.
> > But, year, since register_kprobe() already has similar protection against
> > reusing, register_kretprobe() should do so.
> > 
> > [..]
> >>  	raw_spin_lock_init(&rp->lock);
> >> +
> >> +	if (!hlist_empty(&rp->free_instances))
> >> +		return -EBUSY;
> >> +
> > 
> > Hmm, but can you use check_kprobe_rereg() before raw_spin_lock_init()?
> > If user reuses rp after it starts, rp->lock can already be used.
> 
> Hmm, your advice is very good, we can use check_kprobe_rereg() at 
> the beginning of the register_kretprobe();
> 
> For example:
> 
> int register_kretprobe(struct kretprobe *rp)
> {
>         int ret = 0;
>         struct kretprobe_instance *inst;
>         int i;
>         void *addr;
> 
>         ret = check_kprobe_rereg(&rp->kp);
>         if (ret)
>                 return ret;

Yeah, this looks much better for me :)

Thanks,

> 
> Thank you!
>  
> 


-- 
Masami Hiramatsu <mhiramat@kernel.org>

Re: 答复: [PATCH] kernel/kprobes: add check to avoid kprobe memory leak

[Masami Hiramatsu]

On Tue, 7 Nov 2017 03:43:01 +0000
"chenjiankang (A)" <chenjiankang1@huawei.com> wrote:

> 
> On Thu, 26 Oct 2017 09:22:23 +0800
> chenjiankang <chenjiankang1@huawei.com> wrote:
> 
> > > On Tue, 24 Oct 2017 20:17:02 +0800
> > > JianKang Chen <chenjiankang1@huawei.com> wrote:
> > > 
> > >> The function register_kretprobe is used to initialize a struct 
> > >> kretprobe and allocate a list table for kprobe instance.
> > >> However,in this function, there is a memory leak.
> > >>
> > >> The test case:
> > >>
> > >> static struct kretprobe rp;
> > >> struct  kretprobe *rps[10]={&rp ,&rp ,&rp , &rp ,&rp ,&rp ,&rp ,&rp 
> > >> ,&rp,&rp};
> > > 
> > > What ? this is buggy code. you must not list same kretprobe.
> > > But, year, since register_kprobe() already has similar protection 
> > > against reusing, register_kretprobe() should do so.
> > > 
> > > [..]
> > >>  	raw_spin_lock_init(&rp->lock);
> > >> +
> > >> +	if (!hlist_empty(&rp->free_instances))
> > >> +		return -EBUSY;
> > >> +
> > > 
> > > Hmm, but can you use check_kprobe_rereg() before raw_spin_lock_init()?
> > > If user reuses rp after it starts, rp->lock can already be used.
> > 
> > Hmm, your advice is very good, we can use check_kprobe_rereg() at the 
> > beginning of the register_kretprobe();
> > 
> > For example:
> > 
> > int register_kretprobe(struct kretprobe *rp) {
> >         int ret = 0;
> >         struct kretprobe_instance *inst;
> >         int i;
> >         void *addr;
> > 
> >     +    ret = check_kprobe_rereg(&rp->kp);
> >     +    if (ret)
> >     +            return ret;
> 
> Yeah, this looks much better for me :)
> 
> Thanks,
> 
> > 
> > Thank you!
> >  
> > 
>   Hi Masmi:
>       Any other comment about this patch?

OK, for the issue that you trying to fix as you commented
on this patch is not acceptable, since that is obviously
the module bug, not kretprobe bug. kprobes has no
responsibility to handle such situation.

However, simple re-register safe check can be there.
So, you should change the patch description not to use
such test case.

>From discussion with Zhou Chengming, I realized that the
current check_kprobe_rereg() is not safe for multi-threading,
because there is a chance to re-register kretprobe
2 or more threads concurrently. See below case.

CPU0                   CPU1

check_kprobe_rereg()
INIT_LIST_HEAD()
                       check_kprobe_rereg()
register_kprobe()
                       INIT_LIST_HEAD()
                       register_kprobe()

So, to avoid this case, we have 2 options;
 - Introduce kretprobe_mutex and protect register_kretprobe()
 - Extend kprobe_mutex lock critical section to
   register_kretprobe() and introduce register_kprobe() wrapper
   and __register_kprobe() body function so that
   register_kretprobe() can call raw __register_kprobe().

Former is very simple. I think just for protecting
INIT_LIST_HEAD() from re-register, that one is better and enough.

Thank you,

-- 
Masami Hiramatsu <mhiramat@kernel.org>

[PATCH] kernel/kprobes: add check to avoid kprobe memory leak

[JianKang Chen]

The function register_kretprobe is used to initialize a struct
kretprobe and allocate a list table for kprobe instance.
However,in this function, there is a memory leak.

The test case:

static struct kretprobe rp;
struct  kretprobe *rps[10]={&rp ,&rp ,&rp ,
&rp ,&rp ,&rp ,&rp ,&rp ,&rp,&rp};

static int ret_handler(struct kretprobe_instance *ri, struct pt_regs
*regs)
{
        printk(KERN_DEBUG "ret_handler\n");
        return 0;
}
static int entry_handler(struct kretprobe_instance *ri, struct pt_regs
*regs)
{
        printk(KERN_DEBUG "entry_handler\n");
        return 0;
}
static int __init kretprobe_init(void)
{
        int ret;
        rp.kp.addr = (kprobe_opcode_t *)kallsyms_lookup_name("do_fork");
        rp.handler=ret_handler;
        rp.entry_handler=entry_handler;
        rp.maxactive = 1;

        ret = register_kretprobes(rps,10);
        if (ret < 0)
        {
                return -1;
        }

Result:

unreferenced object 0xffff8012aea35500 (size 64):
  comm "insmod", pid 31966, jiffies 4309153380 (age 111.356s)
  hex dump (first 32 bytes):
    00 00 00 00 00 00 00 00 00 85 37 fc ff 7f ff ff  ..........7.....
    08 55 a3 ae 12 80 ff ff 01 00 00 00 00 00 00 00  .U..............
  backtrace:
    [<ffff8000002cd880>] create_object+0x1e0/0x3f0
    [<ffff800000fa47ac>] kmemleak_alloc+0x6c/0xf0
    [<ffff8000002ac97c>] __kmalloc+0x23c/0x2e0
    [<ffff8000001a6f2c>] register_kretprobe+0x12c/0x350
    [<ffff8000001a7198>] register_kretprobes+0x48/0xa0
    [<ffff7ffffc380058>] 0xffff7ffffc380058
    [<ffff800000084be0>] do_one_initcall+0x120/0x260
    [<ffff800000faa1a4>] do_init_module+0xf0/0x288
    [<ffff8000001736f4>] load_module+0x1824/0x1b50
    [<ffff800000173cc8>] SyS_finit_module+0xc8/0x100
    [<ffff800000084778>] __sys_trace_return+0x0/0x4
    [<ffffffffffffffff>] 0xffffffffffffffff

when there are two same struct kretprobe rp,
the INIT_HLIST_HEAD() will result
in a empty list table rp->free_instances.
The memory leak will happen.
So it needs to check the list table before INIT_HLIST_HEAD().

Reported-and-tested-by: Wen Kang <kangwen1@huawei.com>
Signed-off-by: JianKang Chen <chenjiankang1@huawei.com>
Signed-off-by: xie yisheng <xieyisheng1@huawei.com>
Signed-off-by: Bixuan Cui <cuibixuan@huawei.com>
---
 kernel/kprobes.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/kernel/kprobes.c b/kernel/kprobes.c
index a1606a4..8b3330c 100644
--- a/kernel/kprobes.c
+++ b/kernel/kprobes.c
@@ -1948,6 +1948,10 @@ int register_kretprobe(struct kretprobe *rp)
 #endif
 	}
 	raw_spin_lock_init(&rp->lock);
+
+	if (!hlist_empty(&rp->free_instances))
+		return -EBUSY;
+
 	INIT_HLIST_HEAD(&rp->free_instances);
 	for (i = 0; i < rp->maxactive; i++) {
 		inst = kmalloc(sizeof(struct kretprobe_instance) +
-- 
1.7.12.4