On 2024-10-10, Aleksa Sarai <cyphar@xxxxxxxxxx> wrote:
> On 2024-10-09, Jonathan Corbet <corbet@xxxxxxx> wrote:
> > luca.boccassi@xxxxxxxxx writes:
> >
> > > As discussed at LPC24, add an ioctl with an extensible struct
> > > so that more parameters can be added later if needed. Start with
> > > returning pid/tgid/ppid and creds unconditionally, and cgroupid
> > > optionally.
> >
> > I was looking this over, and a couple of questions came to mind...
> >
> > > Signed-off-by: Luca Boccassi <luca.boccassi@xxxxxxxxx>
> > > ---
> >
> > [...]
> >
> > > diff --git a/fs/pidfs.c b/fs/pidfs.c
> > > index 80675b6bf884..15cdc7fe4968 100644
> > > --- a/fs/pidfs.c
> > > +++ b/fs/pidfs.c
> > > @@ -2,6 +2,7 @@
> > > #include <linux/anon_inodes.h>
> > > #include <linux/file.h>
> > > #include <linux/fs.h>
> > > +#include <linux/cgroup.h>
> > > #include <linux/magic.h>
> > > #include <linux/mount.h>
> > > #include <linux/pid.h>
> > > @@ -114,6 +115,83 @@ static __poll_t pidfd_poll(struct file *file, struct poll_table_struct *pts)
> > > return poll_flags;
> > > }
> > >
> > > +static long pidfd_info(struct task_struct *task, unsigned int cmd, unsigned long arg)
> > > +{
> > > + struct pidfd_info __user *uinfo = (struct pidfd_info __user *)arg;
> > > + size_t usize = _IOC_SIZE(cmd);
> > > + struct pidfd_info kinfo = {};
> > > + struct user_namespace *user_ns;
> > > + const struct cred *c;
> > > + __u64 request_mask;
> > > +
> > > + if (!uinfo)
> > > + return -EINVAL;
> > > + if (usize < sizeof(struct pidfd_info))
> > > + return -EINVAL; /* First version, no smaller struct possible */
> > > +
> > > + if (copy_from_user(&request_mask, &uinfo->request_mask, sizeof(request_mask)))
> > > + return -EFAULT;
> >
> > You don't check request_mask for unrecognized flags, so user space will
> > not get an error if it puts random gunk there. That, in turn, can make
> > it harder to add new options in the future.
>
> In fairness, this is how statx works and statx does this to not require
> syscall retries to figure out what flags the current kernel supports and
> instead defers that to stx_mask.
>
> However, I think verifying the value is slightly less fragile -- as long
> as we get a cheap way for userspace to check what flags are supported
> (such as CHECK_FIELDS[1]). It would kind of suck if userspace would have
> to do 50 syscalls to figure out what request_mask values are valid.
Unfortunately, we probably need to find a different way to do
CHECK_FIELDS for extensible-struct ioctls because CHECK_FIELDS uses the
top bit in a u64 but we can't set a size that large with ioctl
numbers...
Then again, _IOC_SIZEBITS is 14 so we could easily set the ioctl size to
something >PAGE_SIZE fairly easily at least...
> [1]: https://lore.kernel.org/all/20241010-extensible-structs-check_fields-v3-0-d2833dfe6edd@xxxxxxxxxx/
>
> >
> > > + c = get_task_cred(task);
> > > + if (!c)
> > > + return -ESRCH;
> >
> > [...]
> >
> > > +
> > > + /*
> > > + * 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 and
> > > + * the size value will be set to the size the kernel knows about.
> > > + */
> > > + if (copy_to_user(uinfo, &kinfo, min(usize, sizeof(kinfo))))
> > > + return -EFAULT;
> >
> > Which "size value" are you referring to here; I can't see it.
> >
> > If user space has a bigger struct, should you perhaps zero-fill the part
> > the kernel doesn't know about?
> >
> > > + return 0;
> > > +}
> >
> > Thanks,
> >
> > jon
> >
>
> --
> Aleksa Sarai
> Senior Software Engineer (Containers)
> SUSE Linux GmbH
> <https://www.cyphar.com/>
--
Aleksa Sarai
Senior Software Engineer (Containers)
SUSE Linux GmbH
<https://www.cyphar.com/>
Attachment:
signature.asc
Description: PGP signature
