linux-kernel - Re: A missing check bug in cgroup1

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20210917172857.GB13346@blackbody.suse.cz>
Date:   Fri, 17 Sep 2021 19:28:57 +0200
From:   Michal Koutný <mkoutny@...e.com>
To:     Jinmeng Zhou <jjjinmeng.zhou@...il.com>
Cc:     tj@...nel.org, lizefan@...wei.com, hannes@...xchg.org,
        shenwenbosmile@...il.com, cgroups@...r.kernel.org,
        linux-kernel@...r.kernel.org,
        Alexander Viro <viro@...iv.linux.org.uk>,
        linux-fsdevel@...r.kernel.org
Subject: Re: A missing check bug in cgroup1_reconfigure()

On Thu, Sep 16, 2021 at 03:33:49PM +0800, Jinmeng Zhou <jjjinmeng.zhou@...il.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
>