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

[luca.boccassi@gmail.com]

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 */