Skip to content

Commit

Permalink
tracefs: Reset permissions on remount if permissions are options
Browse files Browse the repository at this point in the history
There's an inconsistency with the way permissions are handled in tracefs.
Because the permissions are generated when accessed, they default to the
root inode's permission if they were never set by the user. If the user
sets the permissions, then a flag is set and the permissions are saved via
the inode (for tracefs files) or an internal attribute field (for
eventfs).

But if a remount happens that specify the permissions, all the files that
were not changed by the user gets updated, but the ones that were are not.
If the user were to remount the file system with a given permission, then
all files and directories within that file system should be updated.

This can cause security issues if a file's permission was updated but the
admin forgot about it. They could incorrectly think that remounting with
permissions set would update all files, but miss some.

For example:

 # cd /sys/kernel/tracing
 # chgrp 1002 current_tracer
 # ls -l
[..]
 -rw-r-----  1 root root 0 May  1 21:25 buffer_size_kb
 -rw-r-----  1 root root 0 May  1 21:25 buffer_subbuf_size_kb
 -r--r-----  1 root root 0 May  1 21:25 buffer_total_size_kb
 -rw-r-----  1 root lkp  0 May  1 21:25 current_tracer
 -rw-r-----  1 root root 0 May  1 21:25 dynamic_events
 -r--r-----  1 root root 0 May  1 21:25 dyn_ftrace_total_info
 -r--r-----  1 root root 0 May  1 21:25 enabled_functions

Where current_tracer now has group "lkp".

 # mount -o remount,gid=1001 .
 # ls -l
 -rw-r-----  1 root tracing 0 May  1 21:25 buffer_size_kb
 -rw-r-----  1 root tracing 0 May  1 21:25 buffer_subbuf_size_kb
 -r--r-----  1 root tracing 0 May  1 21:25 buffer_total_size_kb
 -rw-r-----  1 root lkp     0 May  1 21:25 current_tracer
 -rw-r-----  1 root tracing 0 May  1 21:25 dynamic_events
 -r--r-----  1 root tracing 0 May  1 21:25 dyn_ftrace_total_info
 -r--r-----  1 root tracing 0 May  1 21:25 enabled_functions

Everything changed but the "current_tracer".

Add a new link list that keeps track of all the tracefs_inodes which has
the permission flags that tell if the file/dir should use the root inode's
permission or not. Then on remount, clear all the flags so that the
default behavior of using the root inode's permission is done for all
files and directories.

Link: https://lore.kernel.org/linux-trace-kernel/[email protected]

Cc: [email protected]
Cc: Masami Hiramatsu <[email protected]>
Cc: Mark Rutland <[email protected]>
Cc: Mathieu Desnoyers <[email protected]>
Cc: Andrew Morton <[email protected]>
Fixes: 8186fff ("tracefs/eventfs: Use root and instance inodes as default ownership")
Signed-off-by: Steven Rostedt (Google) <[email protected]>
  • Loading branch information
rostedt committed May 4, 2024
1 parent ee4e037 commit baa23a8
Show file tree
Hide file tree
Showing 3 changed files with 99 additions and 2 deletions.
29 changes: 29 additions & 0 deletions fs/tracefs/event_inode.c
Original file line number Diff line number Diff line change
Expand Up @@ -308,6 +308,35 @@ static const struct file_operations eventfs_file_operations = {
.llseek = generic_file_llseek,
};

/*
* On a remount of tracefs, if UID or GID options are set, then
* the mount point inode permissions should be used.
* Reset the saved permission flags appropriately.
*/
void eventfs_remount(struct tracefs_inode *ti, bool update_uid, bool update_gid)
{
struct eventfs_inode *ei = ti->private;

if (!ei)
return;

if (update_uid)
ei->attr.mode &= ~EVENTFS_SAVE_UID;

if (update_gid)
ei->attr.mode &= ~EVENTFS_SAVE_GID;

if (!ei->entry_attrs)
return;

for (int i = 0; i < ei->nr_entries; i++) {
if (update_uid)
ei->entry_attrs[i].mode &= ~EVENTFS_SAVE_UID;
if (update_gid)
ei->entry_attrs[i].mode &= ~EVENTFS_SAVE_GID;
}
}

/* Return the evenfs_inode of the "events" directory */
static struct eventfs_inode *eventfs_find_events(struct dentry *dentry)
{
Expand Down
65 changes: 64 additions & 1 deletion fs/tracefs/inode.c
Original file line number Diff line number Diff line change
Expand Up @@ -30,20 +30,47 @@ static struct vfsmount *tracefs_mount;
static int tracefs_mount_count;
static bool tracefs_registered;

/*
* Keep track of all tracefs_inodes in order to update their
* flags if necessary on a remount.
*/
static DEFINE_SPINLOCK(tracefs_inode_lock);
static LIST_HEAD(tracefs_inodes);

static struct inode *tracefs_alloc_inode(struct super_block *sb)
{
struct tracefs_inode *ti;
unsigned long flags;

ti = kmem_cache_alloc(tracefs_inode_cachep, GFP_KERNEL);
if (!ti)
return NULL;

spin_lock_irqsave(&tracefs_inode_lock, flags);
list_add_rcu(&ti->list, &tracefs_inodes);
spin_unlock_irqrestore(&tracefs_inode_lock, flags);

return &ti->vfs_inode;
}

static void tracefs_free_inode_rcu(struct rcu_head *rcu)
{
struct tracefs_inode *ti;

ti = container_of(rcu, struct tracefs_inode, rcu);
kmem_cache_free(tracefs_inode_cachep, ti);
}

static void tracefs_free_inode(struct inode *inode)
{
kmem_cache_free(tracefs_inode_cachep, get_tracefs(inode));
struct tracefs_inode *ti = get_tracefs(inode);
unsigned long flags;

spin_lock_irqsave(&tracefs_inode_lock, flags);
list_del_rcu(&ti->list);
spin_unlock_irqrestore(&tracefs_inode_lock, flags);

call_rcu(&ti->rcu, tracefs_free_inode_rcu);
}

static ssize_t default_read_file(struct file *file, char __user *buf,
Expand Down Expand Up @@ -313,6 +340,8 @@ static int tracefs_apply_options(struct super_block *sb, bool remount)
struct tracefs_fs_info *fsi = sb->s_fs_info;
struct inode *inode = d_inode(sb->s_root);
struct tracefs_mount_opts *opts = &fsi->mount_opts;
struct tracefs_inode *ti;
bool update_uid, update_gid;
umode_t tmp_mode;

/*
Expand All @@ -332,6 +361,25 @@ static int tracefs_apply_options(struct super_block *sb, bool remount)
if (!remount || opts->opts & BIT(Opt_gid))
inode->i_gid = opts->gid;

if (remount && (opts->opts & BIT(Opt_uid) || opts->opts & BIT(Opt_gid))) {

update_uid = opts->opts & BIT(Opt_uid);
update_gid = opts->opts & BIT(Opt_gid);

rcu_read_lock();
list_for_each_entry_rcu(ti, &tracefs_inodes, list) {
if (update_uid)
ti->flags &= ~TRACEFS_UID_PERM_SET;

if (update_gid)
ti->flags &= ~TRACEFS_GID_PERM_SET;

if (ti->flags & TRACEFS_EVENT_INODE)
eventfs_remount(ti, update_uid, update_gid);
}
rcu_read_unlock();
}

return 0;
}

Expand Down Expand Up @@ -398,7 +446,22 @@ static int tracefs_d_revalidate(struct dentry *dentry, unsigned int flags)
return !(ei && ei->is_freed);
}

static void tracefs_d_iput(struct dentry *dentry, struct inode *inode)
{
struct tracefs_inode *ti = get_tracefs(inode);

/*
* This inode is being freed and cannot be used for
* eventfs. Clear the flag so that it doesn't call into
* eventfs during the remount flag updates. The eventfs_inode
* gets freed after an RCU cycle, so the content will still
* be safe if the iteration is going on now.
*/
ti->flags &= ~TRACEFS_EVENT_INODE;
}

static const struct dentry_operations tracefs_dentry_operations = {
.d_iput = tracefs_d_iput,
.d_revalidate = tracefs_d_revalidate,
.d_release = tracefs_d_release,
};
Expand Down
7 changes: 6 additions & 1 deletion fs/tracefs/internal.h
Original file line number Diff line number Diff line change
Expand Up @@ -11,8 +11,12 @@ enum {
};

struct tracefs_inode {
struct inode vfs_inode;
union {
struct inode vfs_inode;
struct rcu_head rcu;
};
/* The below gets initialized with memset_after(ti, 0, vfs_inode) */
struct list_head list;
unsigned long flags;
void *private;
};
Expand Down Expand Up @@ -73,6 +77,7 @@ struct dentry *tracefs_end_creating(struct dentry *dentry);
struct dentry *tracefs_failed_creating(struct dentry *dentry);
struct inode *tracefs_get_inode(struct super_block *sb);

void eventfs_remount(struct tracefs_inode *ti, bool update_uid, bool update_gid);
void eventfs_d_release(struct dentry *dentry);

#endif /* _TRACEFS_INTERNAL_H */

0 comments on commit baa23a8

Please sign in to comment.