Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [Medium] tracing/remotes: NULL deref in trace_remote_init_eventfs()
under lockdown
- [Low] tracing/remotes: missing Fixes trailers for
trace_remote_register() leaks
--
commit 02468907e3da178a9b587aedf4931ee736d0fcda
Author: Vincent Donnefort [off-list ref]
tracing/remotes: Release tracefs,eventfs on registration failure
In trace_remote_register(), if registration of events or the init
callback fails, the created tracefs and eventfs directories are leaked.
Release the entire eventfs and tracefs hierarchy on trace_remote
registration failure.
Signed-off-by: Vincent Donnefort <vdonnefort@google.com>
[Severity: Low]
This fixes resource leaks on the trace_remote_register() error paths.
Should it include Fixes: tags for the commits that introduced them:
Fixes: 072529158e60 ("tracing: Add events to trace remotes")
Fixes: bf2ba0f8ca1a ("tracing: Add init callback to trace remotes")
quoted hunk ↗ jump to hunk
diff --git a/kernel/trace/trace_remote.c b/kernel/trace/trace_remote.c
index e8cbbd01964df..10091ff9d42ab 100644
--- a/kernel/trace/trace_remote.c
+++ b/kernel/trace/trace_remote.c
[ ... ]
quoted hunk ↗ jump to hunk
@@ -1374,35 +1395,37 @@ static int trace_remote_init_eventfs(const char *remote_name, struct trace_remot
.callback = remote_event_callback,
}
};
- bool eventfs_create = false;
+ struct eventfs_inode *eventfs_root, *eventfs_subdir, *e;
- if (!eventfs) {
- eventfs = eventfs_create_events_dir("events", remote->dentry, dir_entries,
- ARRAY_SIZE(dir_entries), remote);
- if (IS_ERR(eventfs))
- return PTR_ERR(eventfs);
+ eventfs_root = remote->eventfs_root;
+ eventfs_subdir = remote->eventfs_subdir;
+ if (!eventfs_root) {
+ eventfs_root = eventfs_create_events_dir("events", remote->dentry, dir_entries,
+ ARRAY_SIZE(dir_entries), remote);
+ if (IS_ERR(eventfs_root))
+ return PTR_ERR(eventfs_root);
/*
* Create similar hierarchy as local events even if a single system is supported at
* the moment
*/
- eventfs = eventfs_create_dir(remote_name, eventfs, NULL, 0, NULL);
- if (IS_ERR(eventfs))
- return PTR_ERR(eventfs);
-
- remote->eventfs = eventfs;
- eventfs_create = true;
+ eventfs_subdir = eventfs_create_dir(remote_name, eventfs_root, NULL, 0, NULL);
+ if (IS_ERR(eventfs_subdir)) {
+ eventfs_remove_events_dir(eventfs_root);
+ return PTR_ERR(eventfs_subdir);
+ }
}
[Severity: Medium]
Can this dereference a NULL eventfs_root in eventfs_remove_events_dir() if
kernel lockdown becomes active between trace_remote_init_tracefs() and
trace_remote_init_eventfs()?
When security_locked_down(LOCKDOWN_TRACEFS) is true,
eventfs_create_events_dir() returns NULL rather than an ERR_PTR.
IS_ERR(eventfs_root) does not catch NULL, so eventfs_create_dir() is called
with a NULL parent and returns ERR_PTR(-EINVAL). The new cleanup path then
passes eventfs_root == NULL to eventfs_remove_events_dir(), which
unconditionally dereferences it in get_root_inode().
Checking IS_ERR_OR_NULL(eventfs_root) or !eventfs_root after
eventfs_create_events_dir() would avoid this regression.
--
Sashiko AI review · https://sashiko.dev/#/patchset/20261106180657.2460376-1-vdonnefort@google.com?part=2