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

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

[Paul Moore]

On Wed, Oct 2, 2024 at 10:25 AM <luca.boccassi@gmail.com> wrote:
>
> From: Luca Boccassi <bluca@debian.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.
>
> As discussed at LPC24, add an ioctl with an extensible struct
> so that more parameters can be added later if needed. Start with
> exposing: pid, uid, gid, groupid, security label (the latter was
> requested by the LSM maintainer).
>
> Signed-off-by: Luca Boccassi <bluca@debian.org>
> ---
>  fs/pidfs.c                                    | 61 ++++++++++++++++++-
>  include/uapi/linux/pidfd.h                    | 17 ++++++
>  .../testing/selftests/pidfd/pidfd_open_test.c | 50 ++++++++++++++-
>  3 files changed, 126 insertions(+), 2 deletions(-)

...

> diff --git a/include/uapi/linux/pidfd.h b/include/uapi/linux/pidfd.h
> index 565fc0629fff..bfd0965e01f3 100644
> --- a/include/uapi/linux/pidfd.h
> +++ b/include/uapi/linux/pidfd.h
> @@ -16,6 +16,22 @@
>  #define PIDFD_SIGNAL_THREAD_GROUP      (1UL << 1)
>  #define PIDFD_SIGNAL_PROCESS_GROUP     (1UL << 2)
>
> +/* Flags for pidfd_info. */
> +#define PIDFD_INFO_PID                 (1UL << 0)
> +#define PIDFD_INFO_CREDS                   (1UL << 1)
> +#define PIDFD_INFO_CGROUPID                (1UL << 2)
> +#define PIDFD_INFO_SECURITY_CONTEXT        (1UL << 3)
> +
> +struct pidfd_info {
> +        __u64 request_mask;
> +        __u32 size;
> +        uint pid;
> +        uint uid;
> +        uint gid;
> +        __u64 cgroupid;
> +        char security_context[NAME_MAX];

[NOTE: please CC the LSM list on changes like this]

Thanks Luca :)

With the addition of the LSM syscalls we've created a lsm_ctx struct
(see include/uapi/linux/lsm.h) that properly supports multiple LSMs.
The original char ptr "secctx" approach worked back when only a single
LSM was supported at any given time, but now that multiple LSMs are
supported we need something richer, and it would be good to use this
new struct in any new userspace API.

See the lsm_get_self_attr(2) syscall for an example (defined in
security/lsm_syscalls.c but effectively implemented via
security_getselfattr() in security/security.c).

> +} __packed;
> +
>  #define PIDFS_IOCTL_MAGIC 0xFF
>
>  #define PIDFD_GET_CGROUP_NAMESPACE            _IO(PIDFS_IOCTL_MAGIC, 1)
> @@ -28,5 +44,6 @@
>  #define PIDFD_GET_TIME_FOR_CHILDREN_NAMESPACE _IO(PIDFS_IOCTL_MAGIC, 8)
>  #define PIDFD_GET_USER_NAMESPACE              _IO(PIDFS_IOCTL_MAGIC, 9)
>  #define PIDFD_GET_UTS_NAMESPACE               _IO(PIDFS_IOCTL_MAGIC, 10)
> +#define PIDFD_GET_INFO                        _IOWR(PIDFS_IOCTL_MAGIC, 11, struct pidfd_info)
>
>  #endif /* _UAPI_LINUX_PIDFD_H */

-- 
paul-moore.com

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

[Luca Boccassi]

On Wed, 2 Oct 2024 at 15:48, Paul Moore <paul@paul-moore.com> wrote:
>
> On Wed, Oct 2, 2024 at 10:25 AM <luca.boccassi@gmail.com> wrote:
> >
> > From: Luca Boccassi <bluca@debian.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.
> >
> > As discussed at LPC24, add an ioctl with an extensible struct
> > so that more parameters can be added later if needed. Start with
> > exposing: pid, uid, gid, groupid, security label (the latter was
> > requested by the LSM maintainer).
> >
> > Signed-off-by: Luca Boccassi <bluca@debian.org>
> > ---
> >  fs/pidfs.c                                    | 61 ++++++++++++++++++-
> >  include/uapi/linux/pidfd.h                    | 17 ++++++
> >  .../testing/selftests/pidfd/pidfd_open_test.c | 50 ++++++++++++++-
> >  3 files changed, 126 insertions(+), 2 deletions(-)
>
> ...
>
> > diff --git a/include/uapi/linux/pidfd.h b/include/uapi/linux/pidfd.h
> > index 565fc0629fff..bfd0965e01f3 100644
> > --- a/include/uapi/linux/pidfd.h
> > +++ b/include/uapi/linux/pidfd.h
> > @@ -16,6 +16,22 @@
> >  #define PIDFD_SIGNAL_THREAD_GROUP      (1UL << 1)
> >  #define PIDFD_SIGNAL_PROCESS_GROUP     (1UL << 2)
> >
> > +/* Flags for pidfd_info. */
> > +#define PIDFD_INFO_PID                 (1UL << 0)
> > +#define PIDFD_INFO_CREDS                   (1UL << 1)
> > +#define PIDFD_INFO_CGROUPID                (1UL << 2)
> > +#define PIDFD_INFO_SECURITY_CONTEXT        (1UL << 3)
> > +
> > +struct pidfd_info {
> > +        __u64 request_mask;
> > +        __u32 size;
> > +        uint pid;
> > +        uint uid;
> > +        uint gid;
> > +        __u64 cgroupid;
> > +        char security_context[NAME_MAX];
>
> [NOTE: please CC the LSM list on changes like this]
>
> Thanks Luca :)
>
> With the addition of the LSM syscalls we've created a lsm_ctx struct
> (see include/uapi/linux/lsm.h) that properly supports multiple LSMs.
> The original char ptr "secctx" approach worked back when only a single
> LSM was supported at any given time, but now that multiple LSMs are
> supported we need something richer, and it would be good to use this
> new struct in any new userspace API.
>
> See the lsm_get_self_attr(2) syscall for an example (defined in
> security/lsm_syscalls.c but effectively implemented via
> security_getselfattr() in security/security.c).

Thanks for the review, makes sense to me - I had a look at those
examples but unfortunately it is getting a bit beyond my (very low)
kernel skills, so I've dropped the string-based security_context from
v2 but without adding something else, is there someone more familiar
with the LSM world that could help implementing that side?

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

[Paul Moore]

On Fri, Oct 4, 2024 at 2:48 PM Luca Boccassi <luca.boccassi@gmail.com> wrote:
> On Wed, 2 Oct 2024 at 15:48, Paul Moore <paul@paul-moore.com> wrote:
> > On Wed, Oct 2, 2024 at 10:25 AM <luca.boccassi@gmail.com> wrote:

...

> > [NOTE: please CC the LSM list on changes like this]
> >
> > Thanks Luca :)
> >
> > With the addition of the LSM syscalls we've created a lsm_ctx struct
> > (see include/uapi/linux/lsm.h) that properly supports multiple LSMs.
> > The original char ptr "secctx" approach worked back when only a single
> > LSM was supported at any given time, but now that multiple LSMs are
> > supported we need something richer, and it would be good to use this
> > new struct in any new userspace API.
> >
> > See the lsm_get_self_attr(2) syscall for an example (defined in
> > security/lsm_syscalls.c but effectively implemented via
> > security_getselfattr() in security/security.c).
>
> Thanks for the review, makes sense to me - I had a look at those
> examples but unfortunately it is getting a bit beyond my (very low)
> kernel skills, so I've dropped the string-based security_context from
> v2 but without adding something else, is there someone more familiar
> with the LSM world that could help implementing that side?

We are running a little short on devs/time in LSM land right now so I
guess I'm the only real option (not that I have any time, but you know
how it goes).  If you can put together the ioctl side of things I can
likely put together the LSM side fairly quickly - sound good?

-- 
paul-moore.com

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

[luca.boccassi]

On Sat, 5 Oct 2024 at 17:06, Paul Moore <paul@paul-moore.com> wrote:
>
> On Fri, Oct 4, 2024 at 2:48 PM Luca Boccassi <luca.boccassi@gmail.com> wrote:
> > On Wed, 2 Oct 2024 at 15:48, Paul Moore <paul@paul-moore.com> wrote:
> > > On Wed, Oct 2, 2024 at 10:25 AM <luca.boccassi@gmail.com> wrote:
>
> ...
>
> > > [NOTE: please CC the LSM list on changes like this]
> > >
> > > Thanks Luca :)
> > >
> > > With the addition of the LSM syscalls we've created a lsm_ctx struct
> > > (see include/uapi/linux/lsm.h) that properly supports multiple LSMs.
> > > The original char ptr "secctx" approach worked back when only a single
> > > LSM was supported at any given time, but now that multiple LSMs are
> > > supported we need something richer, and it would be good to use this
> > > new struct in any new userspace API.
> > >
> > > See the lsm_get_self_attr(2) syscall for an example (defined in
> > > security/lsm_syscalls.c but effectively implemented via
> > > security_getselfattr() in security/security.c).
> >
> > Thanks for the review, makes sense to me - I had a look at those
> > examples but unfortunately it is getting a bit beyond my (very low)
> > kernel skills, so I've dropped the string-based security_context from
> > v2 but without adding something else, is there someone more familiar
> > with the LSM world that could help implementing that side?
>
> We are running a little short on devs/time in LSM land right now so I
> guess I'm the only real option (not that I have any time, but you know
> how it goes).  If you can put together the ioctl side of things I can
> likely put together the LSM side fairly quickly - sound good?

Here's a skeleton ioctl, needs lsm-specific fields to be added to the new struct, and filled in the new function:

diff --git a/fs/pidfs.c b/fs/pidfs.c
index 4eaf30873105..30310489f5e1 100644
--- a/fs/pidfs.c
+++ b/fs/pidfs.c
@@ -115,6 +115,35 @@ static __poll_t pidfd_poll(struct file *file, struct poll_table_struct *pts)
        return poll_flags;
 }
 
+static long pidfd_security(struct task_struct *task, unsigned int cmd, unsigned long arg)
+{
+       struct pidfd_security __user *usecurity = (struct pidfd_security __user *)arg;
+       size_t usize = _IOC_SIZE(cmd);
+       struct pidfd_security ksecurity = {};
+       __u64 mask;
+
+       if (!usecurity)
+               return -EINVAL;
+       if (usize < PIDFD_SECURITY_SIZE_VER0)
+               return -EINVAL; /* First version, no smaller struct possible */
+
+       if (copy_from_user(&mask, &usecurity->mask, sizeof(mask)))
+               return -EFAULT;
+
+       // TODO: fill in ksecurity
+
+       /*
+        * If userspace and the kernel have the same struct size it can just
+        * be copied. If userspace provides an older struct, only the bits that
+        * userspace knows about will be copied. If userspace provides a new
+        * struct, only the bits that the kernel knows about will be copied.
+        */
+       if (copy_to_user(usecurity, &ksecurity, min(usize, sizeof(ksecurity))))
+               return -EFAULT;
+
+       return 0;
+}
+
 static long pidfd_info(struct task_struct *task, unsigned int cmd, unsigned long arg)
 {
        struct pidfd_info __user *uinfo = (struct pidfd_info __user *)arg;
@@ -160,7 +189,7 @@ static long pidfd_info(struct task_struct *task, unsigned int cmd, unsigned long
         * the flag only if we can still access it.
         */
        rcu_read_lock();
-       cgrp = task_dfl_cgroup(current);
+       cgrp = task_dfl_cgroup(task);
        if (cgrp) {
                kinfo.cgroupid = cgroup_id(cgrp);
                kinfo.mask |= PIDFD_INFO_CGROUPID;
@@ -209,9 +238,11 @@ static long pidfd_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
        if (!task)
                return -ESRCH;
 
-       /* Extensible IOCTL that does not open namespace FDs, take a shortcut */
+       /* Extensible IOCTLs that do not open namespace FDs, take a shortcut */
        if (_IOC_NR(cmd) == _IOC_NR(PIDFD_GET_INFO))
                return pidfd_info(task, cmd, arg);
+       if (_IOC_NR(cmd) == _IOC_NR(PIDFD_GET_SECURITY))
+               return pidfd_security(task, cmd, arg);
 
        if (arg)
                return -EINVAL;
diff --git a/include/uapi/linux/pidfd.h b/include/uapi/linux/pidfd.h
index 4540f6301b8c..0658880a9089 100644
--- a/include/uapi/linux/pidfd.h
+++ b/include/uapi/linux/pidfd.h
@@ -65,6 +65,14 @@ struct pidfd_info {
        __u32 spare0[1];
 };
 
+#define PIDFD_SECURITY_SIZE_VER0               8 /* sizeof first published struct */
+
+struct pidfd_security {
+       /* This mask follows the same rules as pidfd_info.mask. */
+       __u64 mask;
+       // TODO: add data fields and flags and increase size defined above
+};
+
 #define PIDFS_IOCTL_MAGIC 0xFF
 
 #define PIDFD_GET_CGROUP_NAMESPACE            _IO(PIDFS_IOCTL_MAGIC, 1)
@@ -78,5 +86,6 @@ struct pidfd_info {
 #define PIDFD_GET_USER_NAMESPACE              _IO(PIDFS_IOCTL_MAGIC, 9)
 #define PIDFD_GET_UTS_NAMESPACE               _IO(PIDFS_IOCTL_MAGIC, 10)
 #define PIDFD_GET_INFO                        _IOWR(PIDFS_IOCTL_MAGIC, 11, struct pidfd_info)
+#define PIDFD_GET_SECURITY                    _IOWR(PIDFS_IOCTL_MAGIC, 12, struct pidfd_security)
 
 #endif /* _UAPI_LINUX_PIDFD_H */

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

[Luca Boccassi]

On Wed, 23 Oct 2024 at 00:45, <luca.boccassi@gmail.com> wrote:
>
> On Sat, 5 Oct 2024 at 17:06, Paul Moore <paul@paul-moore.com> wrote:
> >
> > On Fri, Oct 4, 2024 at 2:48 PM Luca Boccassi <luca.boccassi@gmail.com> wrote:
> > > On Wed, 2 Oct 2024 at 15:48, Paul Moore <paul@paul-moore.com> wrote:
> > > > On Wed, Oct 2, 2024 at 10:25 AM <luca.boccassi@gmail.com> wrote:
> >
> > ...
> >
> > > > [NOTE: please CC the LSM list on changes like this]
> > > >
> > > > Thanks Luca :)
> > > >
> > > > With the addition of the LSM syscalls we've created a lsm_ctx struct
> > > > (see include/uapi/linux/lsm.h) that properly supports multiple LSMs.
> > > > The original char ptr "secctx" approach worked back when only a single
> > > > LSM was supported at any given time, but now that multiple LSMs are
> > > > supported we need something richer, and it would be good to use this
> > > > new struct in any new userspace API.
> > > >
> > > > See the lsm_get_self_attr(2) syscall for an example (defined in
> > > > security/lsm_syscalls.c but effectively implemented via
> > > > security_getselfattr() in security/security.c).
> > >
> > > Thanks for the review, makes sense to me - I had a look at those
> > > examples but unfortunately it is getting a bit beyond my (very low)
> > > kernel skills, so I've dropped the string-based security_context from
> > > v2 but without adding something else, is there someone more familiar
> > > with the LSM world that could help implementing that side?
> >
> > We are running a little short on devs/time in LSM land right now so I
> > guess I'm the only real option (not that I have any time, but you know
> > how it goes).  If you can put together the ioctl side of things I can
> > likely put together the LSM side fairly quickly - sound good?
>
> Here's a skeleton ioctl, needs lsm-specific fields to be added to the new struct, and filled in the new function:

Forgot to mention, this is based on the vfs.pidfs branch of
https://git.kernel.org/pub/scm/linux/kernel/git/vfs/vfs.git

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

[Paul Moore]

On Tue, Oct 22, 2024 at 7:56 PM Luca Boccassi <luca.boccassi@gmail.com> wrote:
> On Wed, 23 Oct 2024 at 00:45, <luca.boccassi@gmail.com> wrote:
> > On Sat, 5 Oct 2024 at 17:06, Paul Moore <paul@paul-moore.com> wrote:
> > > On Fri, Oct 4, 2024 at 2:48 PM Luca Boccassi <luca.boccassi@gmail.com> wrote:
> > > > On Wed, 2 Oct 2024 at 15:48, Paul Moore <paul@paul-moore.com> wrote:
> > > > > On Wed, Oct 2, 2024 at 10:25 AM <luca.boccassi@gmail.com> wrote:

...

> > > We are running a little short on devs/time in LSM land right now so I
> > > guess I'm the only real option (not that I have any time, but you know
> > > how it goes).  If you can put together the ioctl side of things I can
> > > likely put together the LSM side fairly quickly - sound good?
> >
> > Here's a skeleton ioctl, needs lsm-specific fields to be added to the new struct, and filled in the new function:
>
> Forgot to mention, this is based on the vfs.pidfs branch of
> https://git.kernel.org/pub/scm/linux/kernel/git/vfs/vfs.git

Thanks.  I'll take a closer look at this next week.  In the meantime,
do you have this patch in a tree somewhere publicly fetchable?

-- 
paul-moore.com

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

[Luca Boccassi]

On Fri, 25 Oct 2024 at 00:14, Paul Moore <paul@paul-moore.com> wrote:
>
> On Tue, Oct 22, 2024 at 7:56 PM Luca Boccassi <luca.boccassi@gmail.com> wrote:
> > On Wed, 23 Oct 2024 at 00:45, <luca.boccassi@gmail.com> wrote:
> > > On Sat, 5 Oct 2024 at 17:06, Paul Moore <paul@paul-moore.com> wrote:
> > > > On Fri, Oct 4, 2024 at 2:48 PM Luca Boccassi <luca.boccassi@gmail.com> wrote:
> > > > > On Wed, 2 Oct 2024 at 15:48, Paul Moore <paul@paul-moore.com> wrote:
> > > > > > On Wed, Oct 2, 2024 at 10:25 AM <luca.boccassi@gmail.com> wrote:
>
> ...
>
> > > > We are running a little short on devs/time in LSM land right now so I
> > > > guess I'm the only real option (not that I have any time, but you know
> > > > how it goes).  If you can put together the ioctl side of things I can
> > > > likely put together the LSM side fairly quickly - sound good?
> > >
> > > Here's a skeleton ioctl, needs lsm-specific fields to be added to the new struct, and filled in the new function:
> >
> > Forgot to mention, this is based on the vfs.pidfs branch of
> > https://git.kernel.org/pub/scm/linux/kernel/git/vfs/vfs.git
>
> Thanks.  I'll take a closer look at this next week.  In the meantime,
> do you have this patch in a tree somewhere publicly fetchable?

I've pushed it here: https://github.com/bluca/linux/tree/pidfd_ioctl_lsm