linux-kernel - Re: [PATCH] cgroup: Fix crash with CLONE_INTO

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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <CAJD7tkaQSMSmrb3Nt17-NPAPkvEoUp5tJBg8e4UYn0eU6x-Gqw@mail.gmail.com>
Date:   Sun, 9 Oct 2022 11:42:38 -0700
From:   Yosry Ahmed <yosryahmed@...gle.com>
To:     "Christian A. Ehrhardt" <lk@...e.de>,
        Andrii Nakryiko <andrii@...nel.org>,
        Alexei Starovoitov <ast@...nel.org>,
        Martin KaFai Lau <kafai@...com>, bpf <bpf@...r.kernel.org>
Cc:     Christian Brauner <brauner@...nel.org>, Tejun Heo <tj@...nel.org>,
        syzbot <syzbot+534ee3d24c37c411f37f@...kaller.appspotmail.com>,
        gregkh@...uxfoundation.org,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        syzkaller-bugs@...glegroups.com
Subject: Re: [PATCH] cgroup: Fix crash with CLONE_INTO_CGROUP and v1 cgroups

On Sun, Oct 9, 2022 at 6:10 AM Christian A. Ehrhardt <lk@...e.de> wrote:
>
>
> Since commit f3a2aebdd6, Version 1 cgroups no longer cause an
> error when used with CLONE_INTO_CGROUP. However, the permission
> checks performed during clone assume a Version 2 cgroup.
>
> Restore the error check for V1 cgroups in the clone() path.
>
> Reported-by: syzbot+534ee3d24c37c411f37f@...kaller.appspotmail.com
> Link: https://lore.kernel.org/lkml/000000000000385cbf05ea3f1862@google.com/
> Fixes: f3a2aebdd6 ("cgroup: enable cgroup_get_from_file() on cgroup1")

Thanks for fixing this, and sorry if this caused a mess.

cgroup_get_from_file() independently seemed like it can support
cgroup1, I didn't realize that some of the callers depend on the fact
that it only supports cgroup2.

+Andrii Nakryiko +Alexei Starovoitov +Martin KaFai Lau +bpf
I wonder if BPF users have this dependency. Does cgroup_bpf_attach()
also depend on cgroup_get_from_fd() (which calls
cgroup_get_from_file()) eliminating v1 cgroups?

It seems like cgroup storages (and some other places) use cgroup ids.
Collisions can happen in cgroup1 ids so I am assuming we want to add a
check there as well. Perhaps in cgroup_bpf_attach() ?

I can send a patch for this if that's the case.

> Signed-off-by: Christian A. Ehrhardt <lk@...e.de>
> ---
>  kernel/cgroup/cgroup.c | 5 +++++
>  1 file changed, 5 insertions(+)
>
> diff --git a/kernel/cgroup/cgroup.c b/kernel/cgroup/cgroup.c
> index b6e3110b3ea7..f7fc3afa88c1 100644
> --- a/kernel/cgroup/cgroup.c
> +++ b/kernel/cgroup/cgroup.c
> @@ -6244,6 +6244,11 @@ static int cgroup_css_set_fork(struct kernel_clone_args *kargs)
>                 goto err;
>         }
>
> +       if (!cgroup_on_dfl(dst_cgrp)) {
> +               ret = -EBADF;
> +               goto err;
> +       }
> +
>         if (cgroup_is_dead(dst_cgrp)) {
>                 ret = -ENODEV;
>                 goto err;
> --
> 2.34.1
>