linux-kernel - Re: [PATCH 1/1] cgroup: limit cgroup psi file writes to processes with CAP_SYS

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: <CAJuCfpERczW1YhEW0fN3p2zrdDj-Ec_pCONH6SQVEwTj0BHYMw@mail.gmail.com>
Date:   Wed, 1 Mar 2023 10:05:36 -0800
From:   Suren Baghdasaryan <surenb@...gle.com>
To:     Michal Hocko <mhocko@...e.com>
Cc:     tj@...nel.org, hannes@...xchg.org, lizefan.x@...edance.com,
        peterz@...radead.org, johunt@...mai.com, quic_sudaraja@...cinc.com,
        cgroups@...r.kernel.org, linux-mm@...ck.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/1] cgroup: limit cgroup psi file writes to processes
 with CAP_SYS_RESOURCE

On Wed, Mar 1, 2023 at 1:47 AM Michal Hocko <mhocko@...e.com> wrote:
>
> On Tue 28-02-23 17:46:51, Suren Baghdasaryan wrote:
> > Currently /proc/pressure/* files can be written only by processes with
> > CAP_SYS_RESOURCE capability to prevent any unauthorized user from
> > creating psi triggers. However no such limitation is required for
> > per-cgroup pressure files. Fix this inconsistency by requiring the same
> > capability for writing per-cgroup psi files.
> >
> > Fixes: 6db12ee0456d ("psi: allow unprivileged users with CAP_SYS_RESOURCE to write psi files")
>
> Is this really a regression from this commit? 6db12ee0456d is changing
> permissions of those files to be world writeable with the
> CAP_SYS_RESOURCE requirement. Permissions of cgroup files is not changed
> and the default mode is 644 (with root as an owner) so only privileged
> processes are allowed without any delegation.

Agree, the Fixes line here is not valid. I will remove it.

>
> I think you should instead construct this slightly differently. The
> ultimate goal is to allow a reasonable delegation after all, no?

Yes.

>
> So keep your current patch and extend it by removing the min timeout
> constrain and justify the change by the necessity of the granularity
> tuning as reported by Sudarshan Rajagopala. If this causes any
> regression then a revert would also return the min timeout constrain
> back and we will have to think about a different approach.

I think adding CAP_SYS_RESOURCE check is needed even if we keep the
min timeout capped like today. Without it one could create multiple
cgroups and add a trigger into each one, therefore creating an
unlimited number of "psimon" kernel threads. At some point I expect
them to affect system performance because even at high polling
intervals they still consume some cpu resources. So, this change I
think is needed regardless of the change to min polling period and I
would suggest keeping them separate.

>
> The consistency with the global case is a valid point only partially
> because different cgroups might have different owners which is not
> usually the case for the global psi interface, right?

Correct.

>
> Makes sense?

Yes but hopefully my argument about keeping this and min period
patches separate is reasonable?
Thanks,
Suren.

> --
> Michal Hocko
> SUSE Labs