Re: [PATCH] eventfs: fix a null pointer access in eventfs_iterate

From: Chi Zhiling
Date: Tue Sep 03 2024 - 22:04:25 EST



On 2024/9/4 03:51, Steven Rostedt wrote:
On Thu, 29 Aug 2024 11:24:36 +0800
Chi Zhiling <chizhiling@xxxxxxx> wrote:

From: Chi Zhiling <chizhiling@xxxxxxxxxx>

We found a null pointer accessing in tracefs[1], the reason is that
the variable 'ei_child' is set to LIST_POISON1, that means the list
was removed in eventfs_remove_rec. so when access the ei_child->is_freed,
the panic triggered.

the linked list is protected by eventfs_mutex in eventfs_remove_rec,
Only writes of the link list is protected by the mutex. Reads are not.

so when we access the list of ei_child in eventfs_iterate, we also need
a mutex_lock in there to avoid eventfs_remove_rec modify the list.
Yes you hit a bug, but no this is *not* the solution!


Signed-off-by: Chi Zhiling <chizhiling@xxxxxxxxxx>
---
fs/tracefs/event_inode.c | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/fs/tracefs/event_inode.c b/fs/tracefs/event_inode.c
index 01e99e98457d..4895ed07376b 100644
--- a/fs/tracefs/event_inode.c
+++ b/fs/tracefs/event_inode.c
@@ -642,6 +642,7 @@ static int eventfs_iterate(struct file *file, struct dir_context *ctx)
/* Subtract the skipped entries above */
c -= min((unsigned int)c, (unsigned int)ei->nr_entries);
+ mutex_unlock(&eventfs_mutex);
This list is protected by SRCU (hence the name of the iterator), if you
need to add a mutex around it, something else is broken.

list_for_each_entry_srcu(ei_child, &ei->children, list,
srcu_read_lock_held(&eventfs_srcu)) {
@@ -659,9 +660,12 @@ static int eventfs_iterate(struct file *file, struct dir_context *ctx)
ino = eventfs_dir_ino(ei_child);
- if (!dir_emit(ctx, name, strlen(name), ino, DT_DIR))
+ if (!dir_emit(ctx, name, strlen(name), ino, DT_DIR)) {
+ mutex_unlock(&eventfs_mutex);
goto out_dec;
+ }
}
+ mutex_unlock(&eventfs_mutex);
ret = 1;
out:
srcu_read_unlock(&eventfs_srcu, idx);
The real fix is:

diff --git a/fs/tracefs/event_inode.c b/fs/tracefs/event_inode.c
index 01e99e98457d..8705c77a9e75 100644
--- a/fs/tracefs/event_inode.c
+++ b/fs/tracefs/event_inode.c
@@ -862,7 +862,7 @@ static void eventfs_remove_rec(struct eventfs_inode *ei, int level)
list_for_each_entry(ei_child, &ei->children, list)
eventfs_remove_rec(ei_child, level + 1);
- list_del(&ei->list);
+ list_del_rcu(&ei->list);
free_ei(ei);
}
Can you test that and let me know if it fixes your issue. I'll just go
ahead and apply it as it is an obvious bug.

Okay, I've tested it and I haven't seen any errors.