A missing check bug in cgroup1_reconfigure()

A missing check bug in cgroup1_reconfigure()

[Jinmeng Zhou]

Dear maintainers,
hi, our team has found a missing check bug on Linux kernel v5.10.7
using static analysis.
There is a checking path where cgroup1_get_tree() calls cgroup1_root_to_use()
to mount cgroup_root after checking capability.
However, another no-checking path exists, cgroup1_reconfigure() calls
trace_cgroup_remount()
to remount without checking capability.
We think there is a missing check bug before mounting cgroup_root in
cgroup1_reconfigure().

Specifically, cgroup1_get_tree() uses ns_capable(ctx->ns->user_ns,
CAP_SYS_ADMIN) to check
the permission before calling the critical function
cgroup1_root_to_use() to mount.

1. // check ns_capable() ////////////////////////////
2. int cgroup1_get_tree(struct fs_context *fc)
3. {
4.  struct cgroup_fs_context *ctx = cgroup_fc2context(fc);
5.  int ret;
6.  /* Check if the caller has permission to mount. */
7.  if (!ns_capable(ctx->ns->user_ns, CAP_SYS_ADMIN))
8.    return -EPERM;
9.  cgroup_lock_and_drain_offline(&cgrp_dfl_root.cgrp);
10. ret = cgroup1_root_to_use(fc);
11. ...
12. }

trace_cgroup_remount() is called to remount cgroup_root in
cgroup1_reconfigure().
However, it lacks the check.
1. int cgroup1_reconfigure(struct fs_context *fc)
2. {
3.  struct cgroup_fs_context *ctx = cgroup_fc2context(fc);
4.  struct kernfs_root *kf_root = kernfs_root_from_sb(fc->root->d_sb);
5.  struct cgroup_root *root = cgroup_root_from_kf(kf_root);
6.  int ret = 0;
7.  u16 added_mask, removed_mask;
8.  ...
9.  trace_cgroup_remount(root);
10. ...
11. }

We find cgroup1_reconfigure() is only used in a variable initialization.
Function cgroup1_get_tree() is also used in this initialization.
Both functions are indirectly called which is hard to trace.
We reasonably consider that the two functions can be equally reached
by the user,
therefore, there is a missing check bug.
1. static const struct fs_context_operations cgroup1_fs_context_ops = {
2. …
3.  .get_tree = cgroup1_get_tree,
4.  .reconfigure = cgroup1_reconfigure,
5. };


Thanks!


Best regards,
Jinmeng Zhou

Re: A missing check bug in cgroup1_reconfigure()

[Michal Koutný]

On Thu, Sep 16, 2021 at 03:33:49PM +0800, Jinmeng Zhou <jjjinmeng.zhou@gmail.com> wrote:
> Dear maintainers,
> hi, our team has found a missing check bug on Linux kernel v5.10.7
> using static analysis.
> There is a checking path where cgroup1_get_tree() calls cgroup1_root_to_use()
> to mount cgroup_root after checking capability.
> However, another no-checking path exists, cgroup1_reconfigure() calls
> trace_cgroup_remount()
> to remount without checking capability.
> We think there is a missing check bug before mounting cgroup_root in
> cgroup1_reconfigure().

Thanks for the report.
AFAICS, the callers of the fs_context_operations callbacks do the checks
themselves, therefore I _think_ even the check in cgroup1_get_tree() is
superfluous (see also commit 23bf1b6be9c2 ("kernfs, sysfs, cgroup,
intel_rdt: Support fs_context")).

But let me CC also VFS folks for confirmation (rest of the message
below).

> Specifically, cgroup1_get_tree() uses ns_capable(ctx->ns->user_ns,
> CAP_SYS_ADMIN) to check
> the permission before calling the critical function
> cgroup1_root_to_use() to mount.
> 
> 1. // check ns_capable() ////////////////////////////
> 2. int cgroup1_get_tree(struct fs_context *fc)
> 3. {
> 4.  struct cgroup_fs_context *ctx = cgroup_fc2context(fc);
> 5.  int ret;
> 6.  /* Check if the caller has permission to mount. */
> 7.  if (!ns_capable(ctx->ns->user_ns, CAP_SYS_ADMIN))
> 8.    return -EPERM;
> 9.  cgroup_lock_and_drain_offline(&cgrp_dfl_root.cgrp);
> 10. ret = cgroup1_root_to_use(fc);
> 11. ...
> 12. }
> 
> trace_cgroup_remount() is called to remount cgroup_root in
> cgroup1_reconfigure().
> However, it lacks the check.
> 1. int cgroup1_reconfigure(struct fs_context *fc)
> 2. {
> 3.  struct cgroup_fs_context *ctx = cgroup_fc2context(fc);
> 4.  struct kernfs_root *kf_root = kernfs_root_from_sb(fc->root->d_sb);
> 5.  struct cgroup_root *root = cgroup_root_from_kf(kf_root);
> 6.  int ret = 0;
> 7.  u16 added_mask, removed_mask;
> 8.  ...
> 9.  trace_cgroup_remount(root);
> 10. ...
> 11. }
> 
> We find cgroup1_reconfigure() is only used in a variable initialization.
> Function cgroup1_get_tree() is also used in this initialization.
> Both functions are indirectly called which is hard to trace.
> We reasonably consider that the two functions can be equally reached
> by the user,
> therefore, there is a missing check bug.
> 1. static const struct fs_context_operations cgroup1_fs_context_ops = {
> 2. …
> 3.  .get_tree = cgroup1_get_tree,
> 4.  .reconfigure = cgroup1_reconfigure,
> 5. };
> 
> 
> Thanks!
> 
> 
> Best regards,
> Jinmeng Zhou
>