Re: [PATCH] cgroup: Fix crash with CLONE_INTO_CGROUP and v1 cgroups

[Yosry Ahmed <yosryahmed@google.com>]

On Sun, Oct 9, 2022 at 6:10 AM Christian A. Ehrhardt <lk@c--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@syzkaller.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@c--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
>