[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <6mjie6enbm5ltei4fsanz64ofukbr4gvss7ywi5sjfjxjxing7@efuhtnqvfawd>
Date: Thu, 27 Jun 2024 11:46:10 +0200
From: Michal Koutný <mkoutny@...e.com>
To: Chen Ridong <chenridong@...wei.com>
Cc: tj@...nel.org, lizefan.x@...edance.com, hannes@...xchg.org,
longman@...hat.com, adityakali@...gle.com, sergeh@...nel.org,
cgroups@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH V3] cgroup/cpuset: Prevent UAF in proc_cpuset_show()
On Wed, Jun 26, 2024 at 09:41:01AM GMT, Chen Ridong <chenridong@...wei.com> wrote:
> An UAF can happen when /proc/cpuset is read as reported in [1].
>
> This can be reproduced by the following methods:
> 1.add an mdelay(1000) before acquiring the cgroup_lock In the
> cgroup_path_ns function.
> 2.$cat /proc/<pid>/cpuset repeatly.
> 3.$mount -t cgroup -o cpuset cpuset /sys/fs/cgroup/cpuset/
> $umount /sys/fs/cgroup/cpuset/ repeatly.
>
> The race that cause this bug can be shown as below:
>
> (umount) | (cat /proc/<pid>/cpuset)
> css_release | proc_cpuset_show
> css_release_work_fn | css = task_get_css(tsk, cpuset_cgrp_id);
> css_free_rwork_fn | cgroup_path_ns(css->cgroup, ...);
> cgroup_destroy_root | mutex_lock(&cgroup_mutex);
> rebind_subsystems |
> cgroup_free_root |
> | // cgrp was freed, UAF
> | cgroup_path_ns_locked(cgrp,..);
Thanks for this breakdown.
> ...
> Fix this problem by using rcu_read_lock in proc_cpuset_show().
> As cgroup root_list is already RCU-safe, css->cgroup is safe.
> This is similar to commit 9067d90006df ("cgroup: Eliminate the
> need for cgroup_mutex in proc_cgroup_show()")
Apologies for misleading you in my previous message about root_list.
As I look better at proc_cpuset_show vs proc_cgroup_show, there's a
difference and task_get_css() doesn't rely on root_list synchronization.
I think it could go like this (with my extra comments)
rcu_read_lock();
spin_lock_irq(&css_set_lock);
css = task_css(tsk, cpuset_cgrp_id); // css is stable wrt task's migration thanks to css_set_lock
cgrp = css->cgroup; // whatever we see here, won't be free'd thanks to RCU lock and cgroup_free_root/kfree_rcu
retval = cgroup_path_ns_locked(cgrp, buf, PATH_MAX,
current->nsproxy->cgroup_ns);
...
Your patch should work thanks to the rcu_read_lock and
cgroup_free_root/kfree_rcu and the `if (css->cgroup)` guard is
unnecessary.
So the patch is a functional fix, the reasoning in commit message is
little off. Not sure if Tejun rebases his for-6.10-fixes (with a
possible v4), full fixup commit for this may not be worthy.
Michal
Download attachment "signature.asc" of type "application/pgp-signature" (229 bytes)
Powered by blists - more mailing lists