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

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

[Cheng Jian]

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.

Signed-off-by: Cheng Jian <cj.chengjian@huawei.com>
---
 kernel/kprobes.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/kernel/kprobes.c b/kernel/kprobes.c
index 2625c24..f1fc921 100644
--- a/kernel/kprobes.c
+++ b/kernel/kprobes.c
@@ -1946,6 +1946,11 @@ int register_kretprobe(struct kretprobe *rp)
 		}
 	}
 
+	/* Return error if it's being re-registered */
+	ret = check_kprobe_rereg(&rp->kp);
+	if (ret)
+		return ret;
+
 	rp->kp.pre_handler = pre_handler_kretprobe;
 	rp->kp.post_handler = NULL;
 	rp->kp.fault_handler = NULL;
-- 
2.7.4

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

[Masami Hiramatsu]

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,

> 
> Signed-off-by: Cheng Jian <cj.chengjian@huawei.com>
> ---
>  kernel/kprobes.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/kernel/kprobes.c b/kernel/kprobes.c
> index 2625c24..f1fc921 100644
> --- a/kernel/kprobes.c
> +++ b/kernel/kprobes.c
> @@ -1946,6 +1946,11 @@ int register_kretprobe(struct kretprobe *rp)
>  		}
>  	}
>  
> +	/* Return error if it's being re-registered */
> +	ret = check_kprobe_rereg(&rp->kp);
> +	if (ret)
> +		return ret;
> +
>  	rp->kp.pre_handler = pre_handler_kretprobe;
>  	rp->kp.post_handler = NULL;
>  	rp->kp.fault_handler = NULL;
> -- 
> 2.7.4
> 


-- 
Masami Hiramatsu <mhiramat@kernel.org>

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

[chengjian (D)]

[-- Attachment #1: Type: text/plain, Size: 9027 bytes --]


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!!!


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.

We have a testcase for it, and I put it in the attachment.


test on ARM64 QEMU

use-after-free call trace：

[ 39.852002] BUG: KASAN: use-after-free in 
pre_handler_kretprobe+0x678/0x6b8 [107/7017]
[ 39.852460] Read of size 8 at addr ffff00002fb35408 by task 
DTS202001190821/251
[ 39.852801]
[ 39.854101] CPU: 6 PID: 251 Comm: DTS202001190821 Tainted: G OE 
5.6.0-rc4+ #8
[ 39.854610] Hardware name: linux,dummy-virt (DT)
[ 39.855316] Call trace:
[ 39.855520] dump_backtrace+0x0/0x390
[ 39.855758] show_stack+0x24/0x30
[ 39.855969] dump_stack+0x118/0x170
[ 39.856179] print_address_description.isra.10+0x74/0x3c0
[ 39.856451] __kasan_report+0x128/0x1d8
[ 39.856660] kasan_report+0xc/0x18
[ 39.856858] __asan_report_load8_noabort+0x18/0x20
[ 39.857132] pre_handler_kretprobe+0x678/0x6b8
[ 39.857377] kprobe_breakpoint_handler+0x108/0x580
[ 39.857622] call_break_hook+0x10c/0x1a0
[ 39.858201] brk_handler+0x28/0x88
[ 39.858503] do_debug_exception+0x138/0x2a8
[ 39.858723] el1_dbg+0x24/0x88
[ 39.858907] el1_sync_handler+0xb4/0x178
[ 39.859120] el1_sync+0x7c/0x100
[ 39.859309] _do_fork+0x0/0x8f8
[ 39.859494] el0_svc_common.constprop.2+0x11c/0x428
[ 39.859735] do_el0_svc+0x4c/0xe8
[ 39.859924] el0_svc+0x20/0x78
[ 39.860109] el0_sync_handler+0x104/0x380
[ 39.860322] el0_sync+0x140/0x180
[ 39.860644]
[ 39.860895] Allocated by task 399:
[ 39.861245] save_stack+0x28/0xc8
[ 39.861474] __kasan_kmalloc.isra.10+0xbc/0xd8
[ 39.861709] kasan_kmalloc+0xc/0x18
[ 39.862371] __kmalloc+0x128/0x2a8
[ 39.862561] register_kretprobe+0x1b0/0x868
[ 39.863834] kretprobe_init+0x8c/0x1000 [testKretprobe_007_1]
[ 39.864215] do_one_initcall+0xbc/0x4b0
[ 39.864471] do_init_module+0x194/0x580
[ 39.864730] load_module+0x4110/0x5870
[ 39.864966] __do_sys_init_module+0x264/0x390
[ 39.865268] __arm64_sys_init_module+0x70/0xa0
[ 39.865541] el0_svc_common.constprop.2+0x11c/0x428
[ 39.866287] do_el0_svc+0x4c/0xe8
[ 39.866522] el0_svc+0x20/0x78
[ 39.866746] el0_sync_handler+0x104/0x380
[ 39.867013] el0_sync+0x140/0x180
[ 39.867325]
[ 39.867486] Freed by task 399:
[ 39.867713] save_stack+0x28/0xc8
[ 39.867948] __kasan_slab_free+0x118/0x180
[ 39.868216] kasan_slab_free+0x10/0x18
[ 39.868470] kfree+0x1a0/0x2e0
[ 39.868691] register_kretprobe+0x598/0x868
[ 39.868986] kretprobe_init+0x8c/0x1000 [testKretprobe_007_1]
[ 39.869370] do_one_initcall+0xbc/0x4b0
[ 39.869637] do_init_module+0x194/0x580
[ 39.870332] load_module+0x4110/0x5870
[ 39.870590] __do_sys_init_module+0x264/0x390
[ 39.870877] __arm64_sys_init_module+0x70/0xa0
[ 39.871168] el0_svc_common.constprop.2+0x11c/0x428
[ 39.871471] do_el0_svc+0x4c/0xe8
[ 39.871705] el0_svc+0x20/0x78
[ 39.871925] el0_sync_handler+0x104/0x380
[ 39.872194] el0_sync+0x140/0x180
[ 39.872421]
[ 39.872592] The buggy address belongs to the object at ffff00002fb35400
[ 39.872592] which belongs to the cache kmalloc-128 of size 128
[ 39.873369] The buggy address is located 8 bytes inside of
[ 39.873369] 128-byte region [ffff00002fb35400, ffff00002fb35480)
[ 39.874572] The buggy address belongs to the page:
[ 39.875295] page:fffffe00009ecd00 refcount:1 mapcount:0 
mapping:ffff00003200fc00 index:0x0 compound_mapcount: 0
[ 39.876782] flags: 0x7fffe0000010200(slab|head)
[ 39.877561] raw: 07fffe0000010200 dead000000000100 dead000000000122 
ffff00003200fc00
[ 39.878554] raw: 0000000000000000 0000000000200020 00000001ffffffff 
0000000000000000
[ 39.879064] page dumped because: kasan: bad access detected
[ 39.879493]
[ 39.879669] Memory state around the buggy address:
[ 39.880300] ffff00002fb35300: 00 00 00 fc fc fc fc fc fc fc fc fc fc fc 
fc fc
[ 39.880772] ffff00002fb35380: fc fc fc fc fc fc fc fc fc fc fc fc fc fc 
fc fc
[ 39.881249] >ffff00002fb35400: fb fb fb fb fb fb fb fb fb fb fb fb fb 
fb fb fb [29/7017]
[ 39.881738]
[ 39.882415] ffff00002fb35480: fc fc fc fc fc fc fc fc fc fc fc fc fc fc 
fc fc
[ 39.882836] ffff00002fb35500: fb fb fb fb fb fb fb fb fb fb fb fb fb fb 
fb fb
[ 39.883300] 
==================================================================
[ 39.883761] Disabling lock debugging due to kernel taint



also with Oops

[ 39.884522] Unable to handle kernel paging request at virtual address 
fbd5400000000024
[ 39.885133] Mem abort info:
[ 39.885377] ESR = 0x96000004
[ 39.886105] EC = 0x25: DABT (current EL), IL = 32 bits
[ 39.886507] SET = 0, FnV = 0
[ 39.886725] EA = 0, S1PTW = 0
[ 39.886950] Data abort info:
[ 39.887180] ISV = 0, ISS = 0x00000004
[ 39.887430] CM = 0, WnR = 0
[ 39.887755] [fbd5400000000024] address between user and kernel address 
ranges
[ 39.888991] Internal error: Oops: 96000004 [#1] SMP
[ 39.890666] Dumping ftrace buffer:
[ 39.892131] (ftrace buffer empty)
[ 39.892724] Modules linked in: testKretprobe_007_1(OE+)
[ 39.893690] CPU: 6 PID: 251 Comm: DTS202001190821 Tainted: G B OE 
5.6.0-rc4+ #8
[ 39.894802] Hardware name: linux,dummy-virt (DT)
[ 39.895197] pstate: 600003c5 (nZCv DAIF -PAN -UAO)
[ 39.895558] pc : pre_handler_kretprobe+0x130/0x6b8
[ 39.895880] lr : pre_handler_kretprobe+0x678/0x6b8
[ 39.896181] sp : ffff00002feef9b0
[ 39.896439] x29: ffff00002feef9b0 x28: 1fffe00005f66a81
[ 39.896845] x27: ffff00002fb34900 x26: 1fffe00005f66a80
[ 39.897214] x25: ffffa0000a442728 x24: 00000000000003
[ 39.897555] x23: ffff00002feefbd0 x22: ffff00002fb35408
[ 39.898279] x21: ffffa0000a442730 x20: ffff00002fb35400
[ 39.898619] x19: ffffa0000a442680 x18: 0000000000000000
[ 39.898955] x17: 0000000000000000 x16: 0000000000000000
[ 39.899290] x15: 0000000000000007 x14: 0000000000000003
[ 39.899624] x13: 0000000000000000 x12: 1ffff4000283803c
[ 39.899961] x11: ffff94000283803c x10: dfffa00000000000
[ 39.900362] x9 : ffff94000283803d x8 : 0000000000000001
[ 39.900700] x7 : ffffa000141c01e7 x6 : ffff94000283803d
[ 39.901032] x5 : ffff94000283803d x4 : ffff94000283803d
[ 39.901389] x3 : ffffa0001016bc3c x2 : 1bd5a00000000024
[ 39.901725] x1 : dead000000000122 x0 : dfffa00000000000
[ 39.902600] Call trace:
[ 39.902864] pre_handler_kretprobe+0x130/0x6b8
[ 39.903177] kprobe_breakpoint_handler+0x108/0x580
[ 39.903492] call_break_hook+0x10c/0x1a0
[ 39.903757] brk_handler+0x28/0x88
[ 39.904004] do_debug_exception+0x138/0x2a8
[ 39.904274] el1_dbg+0x24/0x88
[ 39.904494] el1_sync_handler+0xb4/0x178
[ 39.904756] el1_sync+0x7c/0x100
[ 39.904984] _do_fork+0x0/0x8f8
[ 39.905235] el0_svc_common.constprop.2+0x11c/0x428
[ 39.905553] do_el0_svc+0x4c/0xe8
[ 39.906016] el0_svc+0x20/0x78
[ 39.906435] el0_sync_handler+0x104/0x380
[ 39.906707] el0_sync+0x140/0x180
[ 39.907140] Code: f9400681 d2d40000 f2fbffe0 d343fc22 (38e06840)
[ 39.909096] ---[ end trace 9df6bc2e51e22a6c ]---
[ 39.910142] Kernel panic - not syncing: Fatal exception
[ 39.910992] SMP: stopping secondary CPUs
[ 39.912097] Dumping ftrace buffer:
[ 39.912348] (ftrace buffer empty)
[ 39.912722] Kernel Offset: disabled
[ 39.913317] CPU features: 0x10002,20006000
[ 39.913630] Memory Limit: none
[ 39.914612] ---[ end Kernel panic - not syncing: Fatal exception ]---


Thanks

----Cheng Jian


[-- Attachment #2: testcase.tar.gz --]
[-- Type: application/gzip, Size: 3737 bytes --]

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@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>

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

[chengjian (D)]

On 2020/3/7 17:54, Masami Hiramatsu wrote:
> 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,

OK, I will add the WARN_ON in V2.

Thank you.


----Cheng Jian