Commit 4a7ba45b authored by Tejun Heo's avatar Tejun Heo Committed by Andrew Morton

memcg: fix possible use-after-free in memcg_write_event_control()

memcg_write_event_control() accesses the dentry->d_name of the specified
control fd to route the write call.  As a cgroup interface file can't be
renamed, it's safe to access d_name as long as the specified file is a
regular cgroup file.  Also, as these cgroup interface files can't be
removed before the directory, it's safe to access the parent too.

Prior to 347c4a87 ("memcg: remove cgroup_event->cft"), there was a
call to __file_cft() which verified that the specified file is a regular
cgroupfs file before further accesses.  The cftype pointer returned from
__file_cft() was no longer necessary and the commit inadvertently dropped
the file type check with it allowing any file to slip through.  With the
invarients broken, the d_name and parent accesses can now race against
renames and removals of arbitrary files and cause use-after-free's.

Fix the bug by resurrecting the file type check in __file_cft().  Now that
cgroupfs is implemented through kernfs, checking the file operations needs
to go through a layer of indirection.  Instead, let's check the superblock
and dentry type.

Link: https://lkml.kernel.org/r/Y5FRm/cfcKPGzWwl@slm.duckdns.org
Fixes: 347c4a87 ("memcg: remove cgroup_event->cft")
Signed-off-by: default avatarTejun Heo <tj@kernel.org>
Reported-by: default avatarJann Horn <jannh@google.com>
Acked-by: default avatarRoman Gushchin <roman.gushchin@linux.dev>
Acked-by: default avatarJohannes Weiner <hannes@cmpxchg.org>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Michal Hocko <mhocko@kernel.org>
Cc: Muchun Song <songmuchun@bytedance.com>
Cc: Shakeel Butt <shakeelb@google.com>
Cc: <stable@vger.kernel.org>	[3.14+]
Signed-off-by: default avatarAndrew Morton <akpm@linux-foundation.org>
parent a501788a
...@@ -68,6 +68,7 @@ struct css_task_iter { ...@@ -68,6 +68,7 @@ struct css_task_iter {
struct list_head iters_node; /* css_set->task_iters */ struct list_head iters_node; /* css_set->task_iters */
}; };
extern struct file_system_type cgroup_fs_type;
extern struct cgroup_root cgrp_dfl_root; extern struct cgroup_root cgrp_dfl_root;
extern struct css_set init_css_set; extern struct css_set init_css_set;
......
...@@ -167,7 +167,6 @@ struct cgroup_mgctx { ...@@ -167,7 +167,6 @@ struct cgroup_mgctx {
extern spinlock_t css_set_lock; extern spinlock_t css_set_lock;
extern struct cgroup_subsys *cgroup_subsys[]; extern struct cgroup_subsys *cgroup_subsys[];
extern struct list_head cgroup_roots; extern struct list_head cgroup_roots;
extern struct file_system_type cgroup_fs_type;
/* iterate across the hierarchies */ /* iterate across the hierarchies */
#define for_each_root(root) \ #define for_each_root(root) \
......
...@@ -4832,6 +4832,7 @@ static ssize_t memcg_write_event_control(struct kernfs_open_file *of, ...@@ -4832,6 +4832,7 @@ static ssize_t memcg_write_event_control(struct kernfs_open_file *of,
unsigned int efd, cfd; unsigned int efd, cfd;
struct fd efile; struct fd efile;
struct fd cfile; struct fd cfile;
struct dentry *cdentry;
const char *name; const char *name;
char *endp; char *endp;
int ret; int ret;
...@@ -4885,6 +4886,16 @@ static ssize_t memcg_write_event_control(struct kernfs_open_file *of, ...@@ -4885,6 +4886,16 @@ static ssize_t memcg_write_event_control(struct kernfs_open_file *of,
if (ret < 0) if (ret < 0)
goto out_put_cfile; goto out_put_cfile;
/*
* The control file must be a regular cgroup1 file. As a regular cgroup
* file can't be renamed, it's safe to access its name afterwards.
*/
cdentry = cfile.file->f_path.dentry;
if (cdentry->d_sb->s_type != &cgroup_fs_type || !d_is_reg(cdentry)) {
ret = -EINVAL;
goto out_put_cfile;
}
/* /*
* Determine the event callbacks and set them in @event. This used * Determine the event callbacks and set them in @event. This used
* to be done via struct cftype but cgroup core no longer knows * to be done via struct cftype but cgroup core no longer knows
...@@ -4893,7 +4904,7 @@ static ssize_t memcg_write_event_control(struct kernfs_open_file *of, ...@@ -4893,7 +4904,7 @@ static ssize_t memcg_write_event_control(struct kernfs_open_file *of,
* *
* DO NOT ADD NEW FILES. * DO NOT ADD NEW FILES.
*/ */
name = cfile.file->f_path.dentry->d_name.name; name = cdentry->d_name.name;
if (!strcmp(name, "memory.usage_in_bytes")) { if (!strcmp(name, "memory.usage_in_bytes")) {
event->register_event = mem_cgroup_usage_register_event; event->register_event = mem_cgroup_usage_register_event;
...@@ -4917,7 +4928,7 @@ static ssize_t memcg_write_event_control(struct kernfs_open_file *of, ...@@ -4917,7 +4928,7 @@ static ssize_t memcg_write_event_control(struct kernfs_open_file *of,
* automatically removed on cgroup destruction but the removal is * automatically removed on cgroup destruction but the removal is
* asynchronous, so take an extra ref on @css. * asynchronous, so take an extra ref on @css.
*/ */
cfile_css = css_tryget_online_from_dir(cfile.file->f_path.dentry->d_parent, cfile_css = css_tryget_online_from_dir(cdentry->d_parent,
&memory_cgrp_subsys); &memory_cgrp_subsys);
ret = -EINVAL; ret = -EINVAL;
if (IS_ERR(cfile_css)) if (IS_ERR(cfile_css))
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment