Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
From:   Jinmeng Zhou <jjjinmeng.zhou@gmail.com>
Date:   Thu, 16 Sep 2021 15:33:49 +0800
Message-ID: <CAA-qYXjxht4+GhTjNb0xmr4dLQYDVpDbO1R_FDcWtnsrQC=VNQ@mail.gmail.com>
Subject: A missing check bug in cgroup1_reconfigure()
To:     tj@kernel.org, lizefan@huawei.com, hannes@cmpxchg.org
Cc:     shenwenbosmile@gmail.com, cgroups@vger.kernel.org,
        linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

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 =3D 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 =3D 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 =3D cgroup_fc2context(fc);
4.  struct kernfs_root *kf_root =3D kernfs_root_from_sb(fc->root->d_sb);
5.  struct cgroup_root *root =3D cgroup_root_from_kf(kf_root);
6.  int ret =3D 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 =3D {
2. =E2=80=A6
3.  .get_tree =3D cgroup1_get_tree,
4.  .reconfigure =3D cgroup1_reconfigure,
5. };


Thanks!


Best regards,
Jinmeng Zhou