Re: [PATCH v2 1/4] tracing/user_events: Prepare find/delete for same name events
Hello, kernel test robot noticed "BUG:KASAN:slab-use-after-free_in_user_events_ioctl" on: commit: fecc001d587ceeeb47043c20353f257e3f01b39f ("[PATCH v2 1/4] tracing/user_events: Prepare find/delete for same name events") url: https://github.com/intel-lab-lkp/linux/commits/Beau-Belgrave/tracing-user_events-Prepare-find-delete-for-same-name-events/20240203-031207 patch link: https://lore.kernel.org/all/20240202184449.1674-2-be...@linux.microsoft.com/ patch subject: [PATCH v2 1/4] tracing/user_events: Prepare find/delete for same name events in testcase: kernel-selftests version: kernel-selftests-x86_64-60acb023-1_20230329 with following parameters: group: user_events compiler: gcc-12 test machine: 36 threads 1 sockets Intel(R) Core(TM) i9-10980XE CPU @ 3.00GHz (Cascade Lake) with 32G memory (please refer to attached dmesg/kmsg for entire log/backtrace) If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags | Reported-by: kernel test robot | Closes: https://lore.kernel.org/oe-lkp/202402141240.cc5aba78-oliver.s...@intel.com [ 106.969333][ T2278] BUG: KASAN: slab-use-after-free in user_events_ioctl (kernel/trace/trace_events_user.c:2067 kernel/trace/trace_events_user.c:2401 kernel/trace/trace_events_user.c:2543) [ 106.970079][ T2278] Read of size 8 at addr 88816644ef38 by task abi_test/2278 [ 106.970788][ T2278] [ 106.971058][ T2278] CPU: 2 PID: 2278 Comm: abi_test Not tainted 6.7.0-rc8-1-gfecc001d587c #1 [ 106.971881][ T2278] Hardware name: Gigabyte Technology Co., Ltd. X299 UD4 Pro/X299 UD4 Pro-CF, BIOS F8a 04/27/2021 [ 106.972829][ T2278] Call Trace: [ 106.973185][ T2278] [ 106.973514][ T2278] dump_stack_lvl (lib/dump_stack.c:108) [ 106.973966][ T2278] print_address_description+0x2c/0x3a0 [ 106.974597][ T2278] ? user_events_ioctl (kernel/trace/trace_events_user.c:2067 kernel/trace/trace_events_user.c:2401 kernel/trace/trace_events_user.c:2543) [ 106.975099][ T2278] print_report (mm/kasan/report.c:476) [ 106.975542][ T2278] ? kasan_addr_to_slab (mm/kasan/common.c:35) [ 106.976025][ T2278] ? user_events_ioctl (kernel/trace/trace_events_user.c:2067 kernel/trace/trace_events_user.c:2401 kernel/trace/trace_events_user.c:2543) [ 106.976531][ T2278] kasan_report (mm/kasan/report.c:590) [ 106.976978][ T2278] ? user_events_ioctl (kernel/trace/trace_events_user.c:2067 kernel/trace/trace_events_user.c:2401 kernel/trace/trace_events_user.c:2543) [ 106.977481][ T2278] user_events_ioctl (kernel/trace/trace_events_user.c:2067 kernel/trace/trace_events_user.c:2401 kernel/trace/trace_events_user.c:2543) [ 106.977970][ T2278] __x64_sys_ioctl (fs/ioctl.c:52 fs/ioctl.c:871 fs/ioctl.c:857 fs/ioctl.c:857) [ 106.978441][ T2278] do_syscall_64 (arch/x86/entry/common.c:52 arch/x86/entry/common.c:83) [ 106.978889][ T2278] entry_SYSCALL_64_after_hwframe (arch/x86/entry/entry_64.S:129) [ 106.979462][ T2278] RIP: 0033:0x7f2e121c8b5b [ 106.979907][ T2278] Code: 00 48 89 44 24 18 31 c0 48 8d 44 24 60 c7 04 24 10 00 00 00 48 89 44 24 08 48 8d 44 24 20 48 89 44 24 10 b8 10 00 00 00 0f 05 <89> c2 3d 00 f0 ff ff 77 1c 48 8b 44 24 18 64 48 2b 04 25 28 00 00 All code 0: 00 48 89add%cl,-0x77(%rax) 3: 44 24 18rex.R and $0x18,%al 6: 31 c0 xor%eax,%eax 8: 48 8d 44 24 60 lea0x60(%rsp),%rax d: c7 04 24 10 00 00 00movl $0x10,(%rsp) 14: 48 89 44 24 08 mov%rax,0x8(%rsp) 19: 48 8d 44 24 20 lea0x20(%rsp),%rax 1e: 48 89 44 24 10 mov%rax,0x10(%rsp) 23: b8 10 00 00 00 mov$0x10,%eax 28: 0f 05 syscall 2a:* 89 c2 mov%eax,%edx<-- trapping instruction 2c: 3d 00 f0 ff ff cmp$0xf000,%eax 31: 77 1c ja 0x4f 33: 48 8b 44 24 18 mov0x18(%rsp),%rax 38: 64 fs 39: 48 rex.W 3a: 2b .byte 0x2b 3b: 04 25 add$0x25,%al 3d: 28 00 sub%al,(%rax) ... Code starting with the faulting instruction === 0: 89 c2 mov%eax,%edx 2: 3d 00 f0 ff ff cmp$0xf000,%eax 7: 77 1c ja 0x25 9: 48 8b 44 24 18 mov0x18(%rsp),%rax e: 64 fs f: 48 rex.W 10: 2b .byte 0x2b 11: 04 25 add$0x25,%al 13: 28 00 sub%al,(%rax) ... [ 106.981608][ T2278] RSP: 002b:7ffcb0ba5ed0 EFLAGS: 0246 ORIG_RAX: 0010 [ 106.982385][ T2278] RAX: ffda RBX: 7ffcb0ba6228 RCX: 7f2e121c8b5b [ 106.983128][ T22
[PATCH v2 1/4] tracing/user_events: Prepare find/delete for same name events
The current code for finding and deleting events assumes that there will never be cases when user_events are registered with the same name, but different formats. In the future this scenario will exist to ensure user programs can be updated or modify their events and run different versions of their programs side-by-side without being blocked. This change does not yet allow for multi-format events. If user_events are registered with the same name but different arguments the programs see the same return values as before. This change simply makes it possible to easily accomodate for this in future changes. Update find_user_event() to take in argument parameters and register flags to accomodate future multi-format event scenarios. Have find validate argument matching and return error pointers to cover address in use cases, or allocation errors. Update callers to handle error pointer logic. Move delete_user_event() to use hash walking directly now that find has changed. Delete all events found that match the register name, stop if an error occurs and report back to the user. Update user_fields_match() to cover list_empty() scenarios instead of each callsite doing it now that find_user_event() uses it directly. Signed-off-by: Beau Belgrave --- kernel/trace/trace_events_user.c | 106 +-- 1 file changed, 58 insertions(+), 48 deletions(-) diff --git a/kernel/trace/trace_events_user.c b/kernel/trace/trace_events_user.c index 9365ce407426..0480579ba563 100644 --- a/kernel/trace/trace_events_user.c +++ b/kernel/trace/trace_events_user.c @@ -202,6 +202,8 @@ static struct user_event_mm *user_event_mm_get(struct user_event_mm *mm); static struct user_event_mm *user_event_mm_get_all(struct user_event *user); static void user_event_mm_put(struct user_event_mm *mm); static int destroy_user_event(struct user_event *user); +static bool user_fields_match(struct user_event *user, int argc, + const char **argv); static u32 user_event_key(char *name) { @@ -1493,17 +1495,24 @@ static int destroy_user_event(struct user_event *user) } static struct user_event *find_user_event(struct user_event_group *group, - char *name, u32 *outkey) + char *name, int argc, const char **argv, + u32 flags, u32 *outkey) { struct user_event *user; u32 key = user_event_key(name); *outkey = key; - hash_for_each_possible(group->register_table, user, node, key) - if (!strcmp(EVENT_NAME(user), name)) + hash_for_each_possible(group->register_table, user, node, key) { + if (strcmp(EVENT_NAME(user), name)) + continue; + + if (user_fields_match(user, argc, argv)) return user_event_get(user); + return ERR_PTR(-EADDRINUSE); + } + return NULL; } @@ -1860,6 +1869,9 @@ static bool user_fields_match(struct user_event *user, int argc, struct list_head *head = >fields; int i = 0; + if (argc == 0) + return list_empty(head); + list_for_each_entry_reverse(field, head, link) { if (!user_field_match(field, argc, argv, )) return false; @@ -1880,10 +1892,8 @@ static bool user_event_match(const char *system, const char *event, match = strcmp(EVENT_NAME(user), event) == 0 && (!system || strcmp(system, USER_EVENTS_SYSTEM) == 0); - if (match && argc > 0) + if (match) match = user_fields_match(user, argc, argv); - else if (match && argc == 0) - match = list_empty(>fields); return match; } @@ -1922,11 +1932,11 @@ static int user_event_parse(struct user_event_group *group, char *name, char *args, char *flags, struct user_event **newuser, int reg_flags) { - int ret; - u32 key; struct user_event *user; + char **argv = NULL; int argc = 0; - char **argv; + int ret; + u32 key; /* Currently don't support any text based flags */ if (flags != NULL) @@ -1935,41 +1945,34 @@ static int user_event_parse(struct user_event_group *group, char *name, if (!user_event_capable(reg_flags)) return -EPERM; + if (args) { + argv = argv_split(GFP_KERNEL, args, ); + + if (!argv) + return -ENOMEM; + } + /* Prevent dyn_event from racing */ mutex_lock(_mutex); - user = find_user_event(group, name, ); + user = find_user_event(group, name, argc, (const char **)argv, + reg_flags, ); mutex_unlock(_mutex); - if (user) { - if (args) { - argv = argv_split(GFP_KERNEL, args, );