linux-kernel - Re: [PATCH] pidfd: add ioctl to retrieve pid info

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: <CAHC9VhRaS2Hjx1ao7x3BEURGk1Tb1z5_OHFnpHYa-y=62HuvLg@mail.gmail.com>
Date: Fri, 4 Oct 2024 10:05:23 -0400
From: Paul Moore <paul@...l-moore.com>
To: Christian Brauner <brauner@...nel.org>
Cc: luca.boccassi@...il.com, Jeff Layton <jlayton@...nel.org>, 
	Josef Bacik <josef@...icpanda.com>, Oleg Nesterov <oleg@...hat.com>, linux-kernel@...r.kernel.org, 
	linux-fsdevel@...r.kernel.org
Subject: Re: [PATCH] pidfd: add ioctl to retrieve pid info

On Fri, Oct 4, 2024 at 5:29 AM Christian Brauner <brauner@...nel.org> wrote:
> On Wed, Oct 02, 2024 at 03:24:33PM GMT, luca.boccassi@...il.com wrote:
> > From: Luca Boccassi <bluca@...ian.org>
> >
> > A common pattern when using pid fds is having to get information
> > about the process, which currently requires /proc being mounted,
> > resolving the fd to a pid, and then do manual string parsing of
> > /proc/N/status and friends. This needs to be reimplemented over
> > and over in all userspace projects (e.g.: I have reimplemented
> > resolving in systemd, dbus, dbus-daemon, polkit so far), and
> > requires additional care in checking that the fd is still valid
> > after having parsed the data, to avoid races.
> >
> > Having a programmatic API that can be used directly removes all
> > these requirements, including having /proc mounted.

...

> > +             const struct cred *c = get_task_cred(task);
> > +             if (!c)
> > +                     return -ESRCH;
> > +
> > +             info.uid = from_kuid_munged(current_user_ns(), c->uid);
> > +             info.gid = from_kgid_munged(current_user_ns(), c->gid);
> > +     }
> > +
> > +     if (uinfo.request_mask & PIDFD_INFO_CGROUPID) {
> > +             struct cgroup *cgrp = task_css_check(task, pids_cgrp_id, 1)->cgroup;
> > +             if (!cgrp)
> > +                     return -ENODEV;
> > +
> > +             info.cgroupid = cgroup_id(cgrp);
> > +     }
> > +
> > +     if (uinfo.request_mask & PIDFD_INFO_SECURITY_CONTEXT) {
>
> It would make sense for security information to get a separate ioctl so
> that struct pidfd_info just return simple and fast information and the
> security stuff can include things such as seccomp, caps etc pp.

I'm okay with moving the security related info to a separate ioctl,
but I'd like to strongly request that it be merged at the same time as
the process ID related info.  It can be a separate patch as part of a
single patchset if you want to make the ID patch backport friendly for
distros, but I think we should treat the security related info with
the same importance as the ID info.

> > +struct pidfd_info {
> > +        __u64 request_mask;
> > +        __u32 size;
> > +        uint pid;
>
> The size is unnecessary because it is directly encoded into the ioctl
> command.
>
> > +        uint uid;
> > +        uint gid;
> > +        __u64 cgroupid;
> > +        char security_context[NAME_MAX];
> > +} __packed;
>
> The packed attribute should be unnecessary. The structure should simply
> be correctly padded and should use explicitly sized types:
>
> struct pidfd_info {
>         /* Let userspace request expensive stuff explictly. */
>         __u64 request_mask;
>         /* And let the kernel indicate whether it knows about it. */
>         __u64 result_mask;
>         __u32 pid;
>         __u32 uid;
>         __u32 gid;
>         __u64 cgroup_id;
>         __u32 spare0[1];
> };
>
> I'm not sure what LSM info to be put in there and we can just do it as
> an extension.

See my original response to Luca on October 2nd, you were on the To/CC line.

-- 
paul-moore.com