[PATCH v5] eventfs: Fix the NULL pointer dereference bug in eventfs_remove_rec()
Inject fault while probing btrfs.ko, if kstrdup() fails in
eventfs_prepare_ef() in eventfs_add_dir(), it will return ERR_PTR
to assign file->ef. But the eventfs_remove() check NULL in
trace_module_remove_events(), which causes the below NULL
pointer dereference.
As both Masami and Steven suggest, allocater side should handle the
error carefully and remove it, so fix the places where it failed.
Could not create tracefs 'raid56_write' directory
Btrfs loaded, zoned=no, fsverity=no
Unable to handle kernel NULL pointer dereference at virtual address
001c
Mem abort info:
ESR = 0x9604
EC = 0x25: DABT (current EL), IL = 32 bits
SET = 0, FnV = 0
EA = 0, S1PTW = 0
FSC = 0x04: level 0 translation fault
Data abort info:
ISV = 0, ISS = 0x0004, ISS2 = 0x
CM = 0, WnR = 0, TnD = 0, TagAccess = 0
GCS = 0, Overlay = 0, DirtyBit = 0, Xs = 0
user pgtable: 4k pages, 48-bit VAs, pgdp=000102544000
[001c] pgd=, p4d=
Internal error: Oops: 9604 [#1] PREEMPT SMP
Dumping ftrace buffer:
(ftrace buffer empty)
Modules linked in: btrfs(-) libcrc32c xor xor_neon raid6_pq cfg80211 rfkill
8021q garp mrp stp llc ipv6 [last unloaded: btrfs]
CPU: 15 PID: 1343 Comm: rmmod Tainted: G N 6.5.0+ #40
Hardware name: linux,dummy-virt (DT)
pstate: 8005 (Nzcv daif -PAN -UAO -TCO -DIT -SSBS BTYPE=--)
pc : eventfs_remove_rec+0x24/0xc0
lr : eventfs_remove+0x68/0x1d8
sp : 800082d63b60
x29: 800082d63b60 x28: b84b80ddd00c x27: b84b3054ba40
x26: 0002 x25: 800082d63bf8 x24: b84b8398e440
x23: b84b82af3000 x22: dead0100 x21: dead0122
x20: 800082d63bf8 x19: fff4 x18: b84b82508820
x17: x16: x15: 83bc876a3166
x14: 006d x13: 006d x12:
x11: 0001 x10: 17e0 x9 : 0001
x8 : x7 : x6 : b84b84289804
x5 : x4 : 9696969696969697 x3 : 33a5b7601f38
x2 : x1 : 800082d63bf8 x0 : fff4
Call trace:
eventfs_remove_rec+0x24/0xc0
eventfs_remove+0x68/0x1d8
remove_event_file_dir+0x88/0x100
event_remove+0x140/0x15c
trace_module_notify+0x1fc/0x230
notifier_call_chain+0x98/0x17c
blocking_notifier_call_chain+0x4c/0x74
__arm64_sys_delete_module+0x1a4/0x298
invoke_syscall+0x44/0x100
el0_svc_common.constprop.1+0x68/0xe0
do_el0_svc+0x1c/0x28
el0_svc+0x3c/0xc4
el0t_64_sync_handler+0xa0/0xc4
el0t_64_sync+0x174/0x178
Code: 5400052c a90153b3 aa0003f3 aa0103f4 (f9401400)
---[ end trace ]---
Kernel panic - not syncing: Oops: Fatal exception
SMP: stopping secondary CPUs
Dumping ftrace buffer:
(ftrace buffer empty)
Kernel Offset: 0x384b00c0 from 0x80008000
PHYS_OFFSET: 0xcc5b8000
CPU features: 0x88000203,3c02,1000421b
Memory Limit: none
Rebooting in 1 seconds..
Fixes: 5bdcd5f5331a ("eventfs: Implement removal of meta data from eventfs")
Signed-off-by: Jinjie Ruan
Suggested-by: Masami Hiramatsu (Google)
Suggested-by: Steven Rostedt
---
v5:
- Move the ef below dir to keep the "upside-down x-mas tree" format.
v4:
- Not change the returning error code to NULL and simplify the fix.
v3:
- Fix the places where it failed instead of IS_ERR_OR_NULL()
- Update the commit message.
v2:
- Use IS_ERR_OR_NULL() to check instead of IS_ERR() as suggested.
- Update the commit message.
---
kernel/trace/trace_events.c | 13 +
1 file changed, 9 insertions(+), 4 deletions(-)
diff --git a/kernel/trace/trace_events.c b/kernel/trace/trace_events.c
index ed367d713be0..68b02d6f4fbf 100644
--- a/kernel/trace/trace_events.c
+++ b/kernel/trace/trace_events.c
@@ -2297,6 +2297,7 @@ event_subsystem_dir(struct trace_array *tr, const char
*name,
{
struct event_subsystem *system, *iter;
struct trace_subsystem_dir *dir;
+ struct eventfs_file *ef;
int res;
/* First see if we did not already create this dir */
@@ -2329,13 +2330,14 @@ event_subsystem_dir(struct trace_array *tr, const char
*name,
} else
__get_system(system);
- dir->ef = eventfs_add_subsystem_dir(name, parent);
- if (IS_ERR(dir->ef)) {
+ ef = eventfs_add_subsystem_dir(name, parent);
+ if (IS_ERR(ef)) {
pr_warn("Failed to create system directory %s\n", name);
__put_system(system);
goto out_free;
}
+ dir->ef = ef;
dir->tr = tr;
dir->ref_count = 1;
dir->nr_events = 1;
@@ -2415,6 +2417,7 @@ event_create_dir(struct dentry *parent, struct
trace_event_file *file)
struct trace_event_call *call = file->event_call;
struct eventfs_file *ef_subsystem = NULL;
struct trace_array *tr = file->tr;
+ struct eventfs_file *ef;
const char
Re: [PATCH v5] eventfs: Fix the NULL pointer dereference bug in eventfs_remove_rec()
On Tue, 12 Sep 2023 21:47:52 +0800 Jinjie Ruan wrote: > v5: > - Move the ef below dir to keep the "upside-down x-mas tree" format. That was quick! I'll add it to my queue and start testing it. Thanks Ruan! -- Steve
Re: [PATCH v5] eventfs: Fix the NULL pointer dereference bug in eventfs_remove_rec()
On Tue, 12 Sep 2023 21:47:52 +0800
Jinjie Ruan wrote:
> Inject fault while probing btrfs.ko, if kstrdup() fails in
> eventfs_prepare_ef() in eventfs_add_dir(), it will return ERR_PTR
> to assign file->ef. But the eventfs_remove() check NULL in
> trace_module_remove_events(), which causes the below NULL
> pointer dereference.
>
> As both Masami and Steven suggest, allocater side should handle the
> error carefully and remove it, so fix the places where it failed.
>
> Could not create tracefs 'raid56_write' directory
> Btrfs loaded, zoned=no, fsverity=no
> Unable to handle kernel NULL pointer dereference at virtual address
> 001c
> Mem abort info:
>ESR = 0x9604
>EC = 0x25: DABT (current EL), IL = 32 bits
>SET = 0, FnV = 0
>EA = 0, S1PTW = 0
>FSC = 0x04: level 0 translation fault
> Data abort info:
>ISV = 0, ISS = 0x0004, ISS2 = 0x
>CM = 0, WnR = 0, TnD = 0, TagAccess = 0
>GCS = 0, Overlay = 0, DirtyBit = 0, Xs = 0
> user pgtable: 4k pages, 48-bit VAs, pgdp=000102544000
> [001c] pgd=, p4d=
> Internal error: Oops: 9604 [#1] PREEMPT SMP
> Dumping ftrace buffer:
> (ftrace buffer empty)
> Modules linked in: btrfs(-) libcrc32c xor xor_neon raid6_pq cfg80211 rfkill
> 8021q garp mrp stp llc ipv6 [last unloaded: btrfs]
> CPU: 15 PID: 1343 Comm: rmmod Tainted: G N 6.5.0+ #40
> Hardware name: linux,dummy-virt (DT)
> pstate: 8005 (Nzcv daif -PAN -UAO -TCO -DIT -SSBS BTYPE=--)
> pc : eventfs_remove_rec+0x24/0xc0
> lr : eventfs_remove+0x68/0x1d8
> sp : 800082d63b60
> x29: 800082d63b60 x28: b84b80ddd00c x27: b84b3054ba40
> x26: 0002 x25: 800082d63bf8 x24: b84b8398e440
> x23: b84b82af3000 x22: dead0100 x21: dead0122
> x20: 800082d63bf8 x19: fff4 x18: b84b82508820
> x17: x16: x15: 83bc876a3166
> x14: 006d x13: 006d x12:
> x11: 0001 x10: 17e0 x9 : 0001
> x8 : x7 : x6 : b84b84289804
> x5 : x4 : 9696969696969697 x3 : 33a5b7601f38
> x2 : x1 : 800082d63bf8 x0 : fff4
> Call trace:
> eventfs_remove_rec+0x24/0xc0
> eventfs_remove+0x68/0x1d8
> remove_event_file_dir+0x88/0x100
> event_remove+0x140/0x15c
> trace_module_notify+0x1fc/0x230
> notifier_call_chain+0x98/0x17c
> blocking_notifier_call_chain+0x4c/0x74
> __arm64_sys_delete_module+0x1a4/0x298
> invoke_syscall+0x44/0x100
> el0_svc_common.constprop.1+0x68/0xe0
> do_el0_svc+0x1c/0x28
> el0_svc+0x3c/0xc4
> el0t_64_sync_handler+0xa0/0xc4
> el0t_64_sync+0x174/0x178
> Code: 5400052c a90153b3 aa0003f3 aa0103f4 (f9401400)
> ---[ end trace ]---
> Kernel panic - not syncing: Oops: Fatal exception
> SMP: stopping secondary CPUs
> Dumping ftrace buffer:
> (ftrace buffer empty)
> Kernel Offset: 0x384b00c0 from 0x80008000
> PHYS_OFFSET: 0xcc5b8000
> CPU features: 0x88000203,3c02,1000421b
> Memory Limit: none
> Rebooting in 1 seconds..
>
This looks good to me!
Acked-by: Masami Hiramatsu (Google)
Thanks!
> Fixes: 5bdcd5f5331a ("eventfs: Implement removal of meta data from eventfs")
> Signed-off-by: Jinjie Ruan
> Suggested-by: Masami Hiramatsu (Google)
> Suggested-by: Steven Rostedt
> ---
> v5:
> - Move the ef below dir to keep the "upside-down x-mas tree" format.
> v4:
> - Not change the returning error code to NULL and simplify the fix.
> v3:
> - Fix the places where it failed instead of IS_ERR_OR_NULL()
> - Update the commit message.
> v2:
> - Use IS_ERR_OR_NULL() to check instead of IS_ERR() as suggested.
> - Update the commit message.
> ---
> kernel/trace/trace_events.c | 13 +
> 1 file changed, 9 insertions(+), 4 deletions(-)
>
> diff --git a/kernel/trace/trace_events.c b/kernel/trace/trace_events.c
> index ed367d713be0..68b02d6f4fbf 100644
> --- a/kernel/trace/trace_events.c
> +++ b/kernel/trace/trace_events.c
> @@ -2297,6 +2297,7 @@ event_subsystem_dir(struct trace_array *tr, const char
> *name,
> {
> struct event_subsystem *system, *iter;
> struct trace_subsystem_dir *dir;
> + struct eventfs_file *ef;
> int res;
>
> /* First see if we did not already create this dir */
> @@ -2329,13 +2330,14 @@ event_subsystem_dir(struct trace_array *tr, const
> char *name,
> } else
> __get_system(system);
>
> - dir->ef = eventfs_add_subsystem_dir(name, parent);
> - if (IS_ERR(dir->ef)) {
> + ef = eventfs_add_subsystem_dir(name, parent);
> + if (IS_ERR(ef)) {
> pr_warn("Failed to create system directory %s\n", name);
> __put_system(system);
> goto out_free;
> }
>
> + dir->ef = ef;
> dir->tr = tr;
> dir->ref
