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

[Aleksa Sarai]

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